Skip to content

Conversation

SkySkimmer
Copy link
Contributor

@SkySkimmer SkySkimmer commented Jun 26, 2025

@SkySkimmer SkySkimmer requested a review from a team as a code owner June 26, 2025 14:13
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jun 26, 2025
@SkySkimmer SkySkimmer added request: full CI Use this label when you want your next push to trigger a full CI. kind: user messages Error messages, warnings, etc. labels Jun 26, 2025
@SkySkimmer SkySkimmer added this to the 9.2+rc1 milestone Jun 26, 2025
@coqbot-app coqbot-app bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Jun 26, 2025
@proux01 proux01 self-assigned this Jun 27, 2025
@proux01
Copy link
Contributor

proux01 commented Jun 27, 2025

@JasonGross apparently, fiat-crypto was relying on the bug in its output test, could you please prepare an overlay
I'm doing the stdlib one

@proux01 proux01 added kind: fix This fixes a bug or incorrect documentation. part: notations The notation system. labels Jun 27, 2025
@SkySkimmer SkySkimmer added the needs: discussion Further discussion is needed. label Jun 27, 2025
@SkySkimmer
Copy link
Contributor Author

The failing tests look kinda bad no?

< fun var : API.type -> Type =>
---
> fun var : type (Language.Compilers.base.type.type base) -> Type =>
-fun x : positive => (- Z.pos x~0 + 0)%Z
+fun x : positive => (- Zpos x~0 + 0)%Z

@SkySkimmer
Copy link
Contributor Author

Not sure we want this change

Copy link
Member

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bad idea, and seems to me to be removing a feature rather than fixing a bug. At the very least, it should be possible to indicate at declaration time whether am abbreviation is meant to escape the scope of the module. (Note that an abbreviation whose printing does not escape the module scope is already possible to emulate by having an only parsing abbreviation + an only printing notation that is not an abbreviation.)

@proux01
Copy link
Contributor

proux01 commented Jun 30, 2025

Note that an abbreviation whose printing does not escape the module scope is already possible to emulate by having an only parsing abbreviation + an only printing notation that is not an abbreviation.

That's definitely not an acceptable workaround.

That being said, the fix indeed seem more complicated than initially expected. We may need to think a bit more about this whole abbreviation locality story.

@SkySkimmer SkySkimmer added needs: progress Work in progress: awaiting action from the author. and removed needs: discussion Further discussion is needed. labels Jul 1, 2025
@SkySkimmer
Copy link
Contributor Author

Rocq call: try having standard local/export/global behaviours for the printing rule (with export as default, and current behaviour is close to global)

@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 25, 2025
@SkySkimmer SkySkimmer added request: full CI Use this label when you want your next push to trigger a full CI. and removed needs: progress Work in progress: awaiting action from the author. labels Jul 25, 2025
@SkySkimmer SkySkimmer requested review from a team as code owners July 25, 2025 11:53
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Jul 25, 2025
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 9, 2025
proux01 added a commit to proux01/coq-elpi that referenced this pull request Sep 9, 2025
@proux01 proux01 added the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 9, 2025
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 9, 2025
proux01 added a commit to proux01/rewriter that referenced this pull request Sep 10, 2025
proux01 added a commit to proux01/rewriter that referenced this pull request Sep 10, 2025
@proux01 proux01 added the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 10, 2025
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 10, 2025
@proux01
Copy link
Contributor

proux01 commented Sep 10, 2025

if all that is needed is replacing Notation with #[global] Notation or Global Notation, and then there are no diffs, I'm happy to merge an overlay making this change, and have no objection.

CI confirms this is indeed the case
@JasonGross please merge the rewriter and fiat-crypto overlays and update your review status here

andres-erbsen pushed a commit to mit-plv/rewriter that referenced this pull request Sep 10, 2025
proux01 added a commit to rocq-prover/stdlib that referenced this pull request Sep 10, 2025
JasonGross pushed a commit to mit-plv/fiat-crypto that referenced this pull request Sep 14, 2025
Copy link
Member

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not looked at code changes, but the behavior change seems good

@proux01 proux01 added the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 14, 2025
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Sep 14, 2025
@proux01
Copy link
Contributor

proux01 commented Sep 15, 2025

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 6d63917 into rocq-prover:master Sep 15, 2025
7 checks passed
Copy link
Contributor

coqbot-app bot commented Sep 15, 2025

@proux01: Please take care of the following overlays:

  • 20816-SkySkimmer-abbrev-mode.sh

SkySkimmer added a commit to LPCIC/coq-elpi that referenced this pull request Sep 15, 2025
@SkySkimmer SkySkimmer deleted the abbrev-mod branch September 15, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. kind: user messages Error messages, warnings, etc. part: notations The notation system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abbreviations leak through outer module import
3 participants