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

name-based comparison in new label-shadowing check has likely hygiene issues. #24278

Open
pnkfelix opened this issue Apr 10, 2015 · 10 comments

Comments

Projects
None yet
7 participants
@pnkfelix
Copy link
Member

commented Apr 10, 2015

Spawned off of #24162. Our lifetimes only carry a Name, not an Ident, which means the comparisons for shadowing are only doing name based comparisons.

But macros should be free to introduce labels, and have them be treated as independent due to hygiene.

This bug is believed to only introduce issues where code will cause a warning to be emitted by the new shadowing check when it should be accepted; i.e. there are not any known soundness issues associated with this problem.

(Note: While loop labels themselves are Idents, much of the syntax system does not treat them the same way it treats "normal" variables with respect to e.g. hygiene. For example the syntax::visit system does not invoke visit_ident on loop labels.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2015

It looks like it can't be fixed by a simple hygienic comparison (mtwt::resolve(ident1) == mtwt::resolve(ident2)) because every new label (or binding) gets fresh syntactic context even in the absence of macros:

fn main() {
    'a: loop {} // ctxt == 1
    'b: loop {} // ctxt == 2
    'a: loop {} // ctxt == 3
}

so, two label declarations are never equal if compared hygienically. I'm still studying mtwt and trying to understand what can be done with this issue.

@nrc

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

@petrochenkov just because Idents have different syntax contexts, does not necessarily mean they are not equal. Calling resolve evaluates the name within the syntax context, it is quite possible, that resolve the same name in different contexts gives the same result, for example if the contexts contain renames which don't apply to the name.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

In the example above mtwt::resolve(ident_of_a1) != mtwt::resolve(ident_of_a2) IIRC, so some other comparison has to be used to uncover the shadowing. I spent some time trying to find it, but gave up eventually.

@nrc

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

That seems correct - they don't shadow. The shadowing case is:

'a: loop {
    'a: loop {}
}

I think hygienic equality is what we want, if that doesn't work, there might be a bug in the mtwt implementation.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

According to the code and tests (https://github.com/rust-lang/rust/blob/master/src/test/compile-fail/loops-reject-duplicate-labels.rs) label duplication is supposed to be checked globally in a function, regardless of nesting (at least currently) (not sure why exactly). Some discussion here: #21633

@nrc

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

Heh, well that seems wrong to me, but explains why there is trouble doing hygienic comparison. I don't see an easy solution in this case (it's possible a different hygiene algorithm can fix this, or at least allow us to fix it with an easy-ish hack, but I don't think that counts as easy).

@nrc

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

Also lol at that issue being E-Easy!

@brson

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

Ping @pnkfelix @petrochenkov @nrc still a problem?

@brson brson added the P-low label Apr 11, 2017

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

@brson
Yes, this is still an issue.
This is probably solvable with new macro 2.0 hygiene machinery (cc @jseyfried as well).

@jseyfried

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2017

This should "just work" in with declarative macros 2.0 (#40847) -- I'll add a test.
In particular, lifetimes are now Ident, not just Names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.