syntax_pos::symbol::Symbol::gensym() is incompatible with stable hashing.

@petrochenkov

The current scheme for "gensym"ing names via Symbol::gensym() is rather clever and elegant but unfortunately it defies being hashed in a stable way as is needed for incremental compilation. So far, incr. comp. has erroneously treated differing Symbols with equal string contents as being the same. Now situations are starting to arise where this leads to problems (#48923, #49065).

I consider this a somewhat urgent issue.

cc @rust-lang/compiler, esp. @petrochenkov & @jseyfried

I generally consider gensym problematic and would prefer it if all hygiene was done through Span.
There are maybe a few non-hygiene uses but they should probably be done in other ways.

We should investigate implementing gensyms via SyntaxContext (which means via Span after #49154).
In theory, every situation when symbol != gensym(symbol) makes sense should include hygiene into comparison as well.
If "gensymness" is kept in SyntaxContext side tables somehow, then symbol1 == symbol2 will always be equivalent to string_content(symbol1) == string_content(symbol2), that also looks like a simplification of the mental model to me.

Blocked on removing emulation of hygiene with gensyms (can't emulate gensyms with hygiene if we are doing exactly the opposite thing right now!) (https://github.com/petrochenkov/rust/tree/spident2, in progress), which in its turn is waiting for #49154 and #49718.

Could gensym generate a name with a stable prefix (proc macro crate name + version), a stable ID (proc macro invocation local) and some character that is illegal in names, so noone can name it? Maybe even throw in the stable hash of the code it is invoked upon?

Categorizing as P-high so that this keeps annoying us, even though we don't have anyone assigned to it or actively working on it.

@petrochenkov

We've been coming back to this issue in the @rust-lang/compiler meeting without progress for a few weeks in a row now. It would be great if we could come up with some action items. @petrochenkov , the two blocking PRs you mentioned above have be merged in the meantime. What would be the next steps? Or broader question: Do we know what to do and just need to implement it or are there still open questions about how this can be solved?

@petrochenkov

Thanks for the awesome work so far, @petrochenkov!

The current situation is that we don't use gensym anymore but we are still not hashing things properly: #51492 (comment)

I haven't had time to think about the various solution strategies yet.

visiting for triage. No progress yet. Should consider either reassigning or lowering priority if we are still stalled next week.

The problem remains, but at least we have some kind of assertion that should catch problems related to it. Going to downgrade to P-medium for now.

... but at least we have some kind of assertion that should catch problems related to it

In particular, this one:

rust/src/librustc/ty/query/plumbing.rs

Lines 526 to 535 in fbb6275

    
                   // If the following assertion triggers, it can have two reasons: 
        
                   // 1. Something is wrong with DepNode creation, either here or 
        
                   //    in DepGraph::try_mark_green() 
        
                   // 2. Two distinct query keys get mapped to the same DepNode 
        
                   //    (see for example #48923) 
        
                   assert!(!self.dep_graph.dep_node_exists(&dep_node), 
        
                           "Forcing query with already existing DepNode.\n\ 
        
                               - query-key: {:?}\n\ 
        
                               - dep-node: {:?}", 
        
                           key, dep_node);

I haven't seen this written out explicitly, but expansion/hygiene data can be stored in metadata/incremental-data in a stable way using the same DefPath approach as other things.

The basic thing to encode here is ExpnId and it has a well-defined parent DefIndex (currently available through the Definitions::invocation_parents table).
If multiple expansions share the parent def-path, then, I think, they should be able to just get local disambuguators.

mod m {
    fn foo() { // def-path: m::foo
        // Here's how the closures' `NodeId`s get a stable representation.
        let closure1 = || {}; // m::foo(1)
        let closure2 = || {}; // m::foo(2)

        // Same idea, but for macro invocations and their `ExpnId`s instead of `NodeId`s
        mac1!(); // m::foo!(1)
        mac2!(); // m::foo!(2)
    }
}

If we can encode ExpnId, then we can encode SyntaxContext as well (a chain of ExpnIds), then Span and everything else.
ExpnId is the only ID-like thing in this area, other components of hygiene structures are plain data.

#63919 should be one of few last steps in the removal of gensyms, after that we'll only have hygiene, which should be encodable as described above, if I'm not missing anything.

@eddyb

Hm, making macro invocations a regular DefPath component... I haven't thought about this deeply yet but it might be genius :) Then DefPath could already be assigned during parsing, with macros just adding more of them without conflicts. cc @eddyb @nikomatsakis

@michaelwoerister

@michaelwoerister Yeah, that's also roughly my vision for moving incremental further back, I think I discussed it with @nikomatsakis before.

I was supposed to write about this months ago but other things came up.

In short, there'd be "Def" layer that parsing, expansion and cross-crate metadata would all interact with, with AST/HIR remaining mainly for the intra-definition syntactic categories and miscellaneous syntax.

A first step could be splitting HIR into "HIR Def" and "everything else", but moving def paths by themselves further back would also make sense.

Def paths are currently created during expansion, when a freshly expanded AST fragment is integrated into the partially built AST.
That's already early enough for representing expansions with def paths.

Rollup merge of rust-lang#64623 - matthewjasper:underscore-imports, r…

🎉 🎉 🎉 🎉 🎉

Navigation Menu

Repository navigation

syntax_pos::symbol::Symbol::gensym() is incompatible with stable hashing. #49300

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// If the following assertion triggers, it can have two reasons:
	// 1. Something is wrong with DepNode creation, either here or
	// in DepGraph::try_mark_green()
	// 2. Two distinct query keys get mapped to the same DepNode
	// (see for example #48923)
	assert!(!self.dep_graph.dep_node_exists(&dep_node),
	"Forcing query with already existing DepNode.\n\
	- query-key: {:?}\n\
	- dep-node: {:?}",
	key, dep_node);

syntax_pos::symbol::Symbol::gensym() is incompatible with stable hashing. #49300

Description

Activity

eddyb commented on Mar 23, 2018

petrochenkov commented on Mar 23, 2018

petrochenkov commented on Mar 30, 2018

oli-obk commented on Apr 12, 2018

nikomatsakis commented on May 10, 2018

michaelwoerister commented on May 25, 2018

10 remaining items

michaelwoerister commented on Jul 13, 2018

pnkfelix commented on Aug 2, 2018

nikomatsakis commented on Aug 9, 2018

michaelwoerister commented on Aug 9, 2018

petrochenkov commented on Aug 28, 2019

michaelwoerister commented on Aug 28, 2019

eddyb commented on Aug 28, 2019

petrochenkov commented on Aug 28, 2019

michaelwoerister commented on Oct 21, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions