Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #18657: "Include" objects were not always taken into account in Search #18662

Merged

Conversation

herbelin
Copy link
Member

@herbelin herbelin commented Feb 12, 2024

The fix was simple.

Note that this may imply duplication of results: in the case of a module, we have a result both with the original name and with the included names. But this is a phenomenon which is not only about Include. E.g., Module M := N has similar consequences.

Fixes / closes #18657

  • Added / updated test-suite.
  • Added changelog.

@herbelin herbelin added kind: fix This fixes a bug or incorrect documentation. part: modules The module system of Coq. part: search Search and Locate vernac commands. labels Feb 12, 2024
@herbelin herbelin added this to the 8.20+rc1 milestone Feb 12, 2024
@herbelin herbelin requested a review from a team as a code owner February 12, 2024 07:52
@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 Feb 12, 2024
@herbelin herbelin added the request: full CI Use this label when you want your next push to trigger a full CI. label Feb 12, 2024
@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 Feb 12, 2024
@Villetaneuse
Copy link
Contributor

That was quick! Thank you very much. The duplication is unfortunate but this can be treated at another time. I'm not even sure there is a better solution; after all the lemmas are there with two names so this behaviour is expected.

@proux01 proux01 self-assigned this Feb 13, 2024
@proux01
Copy link
Contributor

proux01 commented Feb 13, 2024

@coqbot run full ci

@proux01
Copy link
Contributor

proux01 commented Feb 13, 2024

The duplication is unfortunate but this can be treated at another time.

Indeed, nothing specific to the current PR. Still, this duplication remains one of the painful issues of Search IMHO. I guess, outputing only the shortest name would be the best solution in case of duplication.

@herbelin
Copy link
Member Author

I guess, outputing only the shortest name would be the best solution in case of duplication.

Probably, the simplest would be to ensure that the Search is done starting from the most recent declarations and that whenever an Include N (for N a module) or Module M:=N is encountered, N is added to the list of modules to later skip.

@proux01
Copy link
Contributor

proux01 commented Feb 13, 2024

There are also Import and Export. Maybe a deduplication mechanism would be enough as a short term solution.

@herbelin
Copy link
Member Author

If I'm not mistaken, the impact of Import and Export on logical declarations is just about how they are made visible in the nametab. Search sees the contents of non-imported modules anyway.

Oh, maybe a simple way is to display only the names whose user name is the same as the canonical name! That could be tried.

@herbelin
Copy link
Member Author

Hum, if we do that, printing only the original "canonical" constants, then this means printing the oldest one, which is probably not the intended, shortly-named, one.

On the other side, if we hide re-aliased module, that's not fully satisfactory either. If we have Module M := N. Module P := N. or Module M := N. Module P := M, we want to give names in P but skip N and M. But that also seems more work.

@proux01
Copy link
Contributor

proux01 commented Feb 13, 2024

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 550dc88 into coq:master Feb 13, 2024
7 checks passed
louiseddp pushed a commit to louiseddp/coq that referenced this pull request Feb 27, 2024
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. part: modules The module system of Coq. part: search Search and Locate vernac commands.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search in a module does not show lemmas obtained when Including a module type
3 participants