Use InternedString instead of Symbol for type parameter types #49266
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors p=1 (since this fixes a regression) |
|
In theory this bug is present since last summer. However, I think it's only trigger but rather new code, since we haven't seen it before and then multiple people ran into it at once. So I don't think we need to backport. |
michaelwoerister
changed the title
|
@michaelwoerister you have to regenerate the ui tests: https://travis-ci.org/rust-lang/rust/builds/356813280#L2484 some of them dump debug info |
src/librustc/ty/mod.rs
Outdated
| "{:?} (addr={:x}, ptr={})", | ||
| slice, | ||
| self.as_ptr() as usize, | ||
| self.len()) |
There was a problem hiding this comment.
If you want to keep this, it should check the verbose flag via ty::tls - also, you meant len instead of ptr, right?
There was a problem hiding this comment.
hmm. that would be verbose indeed. I'd almost rather yet another flag -- but really what we need to do is make {:?} and (today's) -Zverbose equivalent. That is, I had hoped that -Zverbose would just make {} and {:?} do the same thing, not go altering debug to dump yet more information.
There was a problem hiding this comment.
Whoops, that was not supposed to be part of the comment. It's a leftover from debugging. Unless you thing it is generally useful, I'd just remove it.
|
Why the changes to |
src/libsyntax_pos/symbol.rs
Outdated
| @@ -18,6 +18,7 @@ use GLOBALS; | |||
| use serialize::{Decodable, Decoder, Encodable, Encoder}; | |||
| use std::collections::HashMap; | |||
| use std::fmt; | |||
| use std::ops::Deref; | |||
There was a problem hiding this comment.
any particular reason for this change?
There was a problem hiding this comment.
(just efficiency, I guess?)
| @@ -2221,12 +2221,12 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { | |||
|
|
|||
| pub fn mk_param(self, | |||
| index: u32, | |||
| name: Name) -> Ty<'tcx> { | |||
| name: InternedString) -> Ty<'tcx> { | |||
There was a problem hiding this comment.
this change seems fine to me (as discussed on IRC); name should only be used for debug print-outs etc
src/librustc/ty/sty.rs
Outdated
| @@ -57,7 +57,7 @@ pub enum BoundRegion { | |||
| /// | |||
| /// The def-id is needed to distinguish free regions in | |||
| /// the event of shadowing. | |||
| BrNamed(DefId, Name), | |||
| BrNamed(DefId, InternedString), | |||
There was a problem hiding this comment.
Hmm, this could affect code, as these names are compared from time to time. That said, I don't think we support hygiene on lifetime names, right?
The fact that this gives an error suggests I am right.
Maybe @petrochenkov knows.
There was a problem hiding this comment.
Lifetime names are hygienic in macros 2.0 -- https://play.rust-lang.org/?gist=f3524076b1ad62bacb3979a98ebb72b2&version=nightly
There was a problem hiding this comment.
Does it matter this late in the pipeline? I thought hygiene would have been satisfied at this point by assigning different DefIds.
There was a problem hiding this comment.
hmm, you might be right.
There was a problem hiding this comment.
if that is true, then, given that we have the def-id, we could probably remove the InternedString altogether. The fact that we didn't... well, either an accident, or intentional. I remember @eddyb was playing with this stuff.
src/librustc/ty/mod.rs
Outdated
| "{:?} (addr={:x}, ptr={})", | ||
| slice, | ||
| self.as_ptr() as usize, | ||
| self.len()) |
There was a problem hiding this comment.
hmm. that would be verbose indeed. I'd almost rather yet another flag -- but really what we need to do is make {:?} and (today's) -Zverbose equivalent. That is, I had hoped that -Zverbose would just make {} and {:?} do the same thing, not go altering debug to dump yet more information.
I did run into code relying on this when I tried to make |
|
I don't quite understand when exactly these names are hashed (and why this PR fixes the issue), so hashing string contents may be okay here if it's done late enough, but in general using string comparisons/hashing for identifiers like lifetimes or type parameters (or comparisons of pointers to strings, which are equivalent with our implementation of string interner and gensyms) is a way to undermine hygiene. As long as gensyms exist, we should not rely on string comparison/hashing. If gensyms need to be hashed for incremental compilation or something like this, we need to invent some predictable scheme for encoding them instead (e.g. "base" Regarding replacement of
|
Comparision by pointer is for efficiency because I saw things like this: Lines 887 to 894 in 52f7e88 The The PR tries to move away from |
Yes, that's what this PR tries to do: Replace |
|
@michaelwoerister // Does not implement `PartialEq`
struct ResolvedIdent {
// True identity of the identifier, was previously obtained by hygienic resolution
def_id: DefId,
// Purely informational string, should never be compared, but can be e.g. displayed in error messages
info: InternedString,
}My goal is to avoid footguns first of all. I've seen enough times code in later stages of compilation (e.g. pattern exhaustiveness checking) behaving like "what hygiene? let's just compare these two fields by their string contents". |
That's a very good goal to have! How should we proceed here? I think I'll do a minimal version of the fix and close this one. |
|
Le sigh, now I remember why I didn't keep the fix minimal: because it's ugly and requires converting back and forth between I opened #49300 to keep track of the issue. Not sure how to proceed. |
|
Ok, now I think I finally understand what causes the ICE. Since this is a temporary solution, I'm happy with this PR as is. |
This makes |
|
@Zoxc |
michaelwoerister
force-pushed
the
unhygienic-ty
branch
from
aaf19e8
to
1768d4a
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Unstable symbol names probably due to this line: rust/src/librustc_trans_utils/symbol_names.rs Line 197 in 5758c2d |
|
I'm a bit confused as to the status of this PR. Are we waiting to see if we can make it pass travis? :) |
|
I'm trying to do a trimmed down version suitable backporting. This version would only change type parameter names but not regions. |
michaelwoerister
force-pushed
the
unhygienic-ty
branch
from
82bd929
to
697b4d4
Compare
|
OK, so this version fixes the regression reported in #48923. It only changes the |
michaelwoerister
changed the title
|
@eddyb has some reservations about the changes to |
Use InternedString instead of Symbol for type parameter types Reduced alternative to #49266. Let's see if this causes a performance regression.
|
☔ The latest upstream changes (presumably #49045) made this pull request unmergeable. Please resolve the merge conflicts. |
Use InternedString instead of Symbol for type parameter types (2) Reduced alternative to #49266. Let's see if this causes a performance regression.
|
Closing in favor of #49695. |
…akis Use InternedString instead of Symbol for type parameter types (2) Reduced alternative to #49266. Let's see if this causes a performance regression.
This PR addresses the regression in #48923 and fixes this particular instance. However, since
Symbolis still used in other places, there might still be instances where this is a problem. At least, this PR makes sure that we run into an assertion immediately instead of silently corrupting the compiler state.The underlying problem is that we cannot properly hash "gensymed"
Symbols (at least not in a stable way), so that two different symbols, with the same string contents but different interning keys, will result in two different query keys but the sameDepNodebecause the later is based only on the string contents of the symbol.r? @nikomatsakis
This PR makes the
EqandHashimplementations forInternedStringpointer- instead of string-based. It would be great if @eddyb or somebody else from @rust-lang/compiler could double-check the logic here.