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

feat: add namespace utility function #2341

Closed
wants to merge 2 commits into from

Conversation

fgdorais
Copy link
Contributor

It was pointed out in std4#184 that this function is frequently reimplemented and that it may find a better home here.

@digama0
Copy link
Collaborator

digama0 commented Jul 21, 2023

Functions I found implementing this function or a variant:

  • Lean.Elab.mkDeclName
  • Lean.Elab.Command.elabSyntax doesn't use the function, but should (see fix: resolve _root_ in macro, syntax, elab #2239). That PR introduces 4 reimplementations of the function
  • Lean.Elab.Term.elabWithDeclName should also be using the function (basically every use of (← getCurrNamespace) ++ id is suspect)

@fgdorais
Copy link
Contributor Author

I'm wondering if removing the _root_ prefix from the rhs is a bit excessive. Maybe the user should decide whether to remove it afterwards.

@digama0
Copy link
Collaborator

digama0 commented Jul 21, 2023

I think the function should be specced to always remove the _root_ prefix and return a fully qualified name. The use code can put _root_ on the front if needed. It shouldn't add the _root_ always because _root_ prefixed names aren't valid fully qualified names, they are a directive to whatever is processing the name to (suppress additional namespaces and) remove the _root_ before using the name, so you would only want to use that when putting the name back into syntax or similar to be re-processed.

@fgdorais
Copy link
Contributor Author

Should it then also remove _root_ from the namespace prefix, or just assume that it is a proper namespace?

@digama0
Copy link
Collaborator

digama0 commented Jul 21, 2023

Namespaces can't have _root_.

@fgdorais
Copy link
Contributor Author

So it is fine as is, with a small edit to the docs?

@fgdorais
Copy link
Contributor Author

Fixed!

@fgdorais
Copy link
Contributor Author

fgdorais commented Jul 30, 2023

std4#184 has evolved into std4#200, which doesn't currently use this function. I'm not unilaterally closing this since this PR could be useful for other purposes.

@Kha
Copy link
Member

Kha commented Feb 4, 2025

Thanks for the PR. At this point I think it's fair to say it will not be merged by itself but might still be included in other PRs where it makes sense.

@Kha Kha closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants