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.

Sign up for GitHub

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

Jump to bottom

incr.comp.: Possible cross-session instability in DefPath #42550

Closed
michaelwoerister opened this issue Jun 8, 2017 · 3 comments
Closed

incr.comp.: Possible cross-session instability in DefPath #42550

michaelwoerister opened this issue Jun 8, 2017 · 3 comments
Labels
A-incr-comp Area: Incremental compilation A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..)

Comments

@michaelwoerister
Copy link
Member

It seems problematic to me that DefPathData (the components of a DefPath) use Ident for storing their string fields because Ident contains SyntaxContext, which, if I understand it correctly, is a monotonically increasing interning key/ID. As a consequence, changing something somewhere involving a new SyntaxContext might have non-local effects, as the u32 in there possibly gets shifted for all subsequent Idents. This would be unfortunate because avoiding issues of this kind is the reason we introduced DefPath for in the first place.

My question is: Why do we even use Ident here in the first place? Wouldn't InternedString or Symbol be sufficient? DefPath already has a mechanism for disambiguating things that have the same string value (see DisambiguatedDefPathData).

cc @jseyfried and the rest of @rust-lang/compiler
@michaelwoerister michaelwoerister added A-incr-comp Area: Incremental compilation A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Jun 8, 2017
@eddyb
Copy link
Member

eddyb commented Jun 8, 2017

I am confused, I thought it's always been InternedString? Did it get changed accidentally?

@michaelwoerister
Copy link
Member Author

Did it get changed accidentally?

It was introduced in 3eb235b. But I don't think it makes sense for DefPath to interact with macro hygiene. We should just change it back if nothing started to rely on having Ident in the meantime.

@jseyfried, can you tell us what the motivation for this change was (just with respect to DefPathData)?

@nikomatsakis
Copy link
Contributor

I believe it should be InternedString. This should not (I think) affect hygiene.

@michaelwoerister michaelwoerister mentioned this issue Jun 13, 2017
Merged
bors added a commit that referenced this issue Jun 13, 2017
@bors
Auto merge of #42627 - michaelwoerister:no-ident-in-def-path, r=eddyb
03abb1b 
incr.comp.: Don't use Ident in DefPath because that's unstable across compilation sessions

Fixes #42550.

cc @jseyfried @nikomatsakis

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-incr-comp Area: Incremental compilation A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@eddyb @nikomatsakis @michaelwoerister