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

Ignore parent locals when resolving in local items #112142

Closed
wants to merge 1 commit into from

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented May 31, 2023

An attempt at fixing #33118

I do want more tests for this, if there are ideas do tell as I couldn't immediately come up with more interesting cases. I'm mainly worried that there might be certain cases where we get multiple errors now.

@rustbot
Copy link
Collaborator

rustbot commented May 31, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 31, 2023
@petrochenkov
Copy link
Contributor

First of all, I'm not sure this is something that needs to be fixed.
I'll refresh #33118 in memory and see how it fits into the current name resolution story.

@Veykril
Copy link
Member Author

Veykril commented May 31, 2023

Assumed this might be something that doesn't necessarily want fixing (but it was small enough that it didn't take much time to try to address). If the consensus will be to not fix it I'll look into possibly adjusting the diagnostics then (as they could probably get some love here).

@petrochenkov
Copy link
Contributor

I think my position now is basically opposite to what I wrote in #33118 back in 2016.

I think we should keep the existing behavior, and also continue supporting "local variable capturing" in macros.

The current rules are very simple - there's only one "hard" barrier for all names, mod items.
There are also numerous "soft" barriers restricting use of some names from some contexts, including but not limited to using outer local variables from inner functions.

"Hard" barrier means that we just stop the lookup once reaching a mod item.
"Soft" barrier means that we see the name, but report an error if that name is found during lookup.

For example, local variables also cannot be used in constant evaluation contexts, and it is also enforced through soft barriers.
Should we also convert it to a hard barrier? Probably not.
Generic parameters from outer scopes also usually cannot be used in inner scopes, but in some cases they can.
Should we also conditionally introduce hard barriers for generic parameters? I'd also say no.
Some soft barriers are (or at least were) added as stubs for unimplemented functionality, and we may remove them in the future.
Removing a soft barrier simply removes an error, but removing a hard barrier may be a silent change in behavior.

The examples listed in #112142 are all relatively esoteric and do not pose a pressing issue.
So in the end I wouldn't want to trade the simplicity and predictability of rules for making them match expectations of some subset of people.

@petrochenkov
Copy link
Contributor

As for #33118 (comment) in macros, it's not only actually useful in practice, but it also matches the intuition of names being "resolved at definition site".

Right now local variables can be resolved at macro definition site too, similarly to any other names.
But if we introduce the macro isolation as discussed in #33118, then we'd no longer resolve names "as if at definition site".

Inability of using outer local variables from functions can be seen as an "unfortunate implementation limitation" from a certain angle, we just cannot codegen it properly, but in other more dynamic and garbage collected languages it can actually be supported.
In case of macros we can indeed codegen it properly, so why not support it.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 8, 2023

I cases like #33118 (comment) we could try "fortifying" the rules by producing errors instead of silently falling back to creating fresh local variables.

E.g.

  • if fresh binding candidate resolves to a name shadowable with a fresh binding (e.g. a local variable of a function),
  • ... but that name is not usable due to being behind a soft barrier,
  • then produce the corresponding soft barrier error instead of falling back to a fresh binding.

This "fortification" would need to go through crater testing, of course.

EDIT: But even if we cannot fortify it this way it's not a big deal, it's also an esoteric case and it can be left as is.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2023
@Veykril
Copy link
Member Author

Veykril commented Jun 12, 2023

I think we should keep the existing behavior, and also continue supporting #33118 (comment) in macros.

That we should definitely keep I think as well (I did not check whether the approach here breaks that which I definitely should have).

Anyways, the decision to not pursue this is fine with me, I'll take a look at maybe improving the diagnostics here instead then.

@Dylan-DPC
Copy link
Member

Closing this as it was decided not to pursue this in the above discussion.

@Dylan-DPC Dylan-DPC closed this Aug 15, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants