ASTs aren't Equatable?

[dabrahams]

Maybe I knew this and understood why at one point but now I find it really problematic.
Sure makes it hard to validate serialization!
This failure suggests ASTs don't make a successful round-trip. The failure is an unexpected nil optional in

self.decl = ast.coreTrait("ExpressibleByFloatLiteral")!.decl

[kyouko-taiga]

I guess the failure occurred because deserialization doesn't assign AST.coreTraits

[dabrahams]

If all the parts are Codable it should just serialize/deserialize automatically. But is there a reason ASTs can't be equatable?

[kyouko-taiga]

If all the parts are Codable it should just serialize/deserialize automatically.

No because coreTraits is outside of AST.Storage. The bug is my fault; I wrote the property there because putting it in storage caused the undefined behavior we observed after Lucian's change, but at that time I didn't understand what was going wrong and moved coreTraits in AST without thinking about the consequences for Codable.

But is there a reason ASTs can't be equatable?

No, but implementing Equatable should require some care if we want something more powerful than representation equality. It would be reasonable to expect that the order of the elements in AST.nodes is not significant. That's a throwback to the whole Movable : Equatable discussion 😅

[kyouko-taiga]

Note that I think we'll have to accept that two ASTs parsed from the same sources might have different representations if we want to ever support incremental compilation.

[dabrahams]

No because coreTraits is outside of AST.Storage

Oh, and we have customized serialization. Presumably you could have left it outside Storage with the appropriate adjustments to serialization.

The bug is my fault; I wrote the property there because putting it in storage caused the undefined behavior we observed after Lucian's change, but at that time I didn't understand what was going wrong [emphasis mine]

Wow, I'm so lost now.

I'm only aware of one case of observed UB (which could be suppressed by not writing a shared cache or not parallelizing type checking), and didn't realize it was associated with a change Lucian made, or (obviously) which change that would have been, and as far as I knew until reading this, it was still a mystery. Have you solved it? What was the issue? Was the property being inside storage actually the cause of the problem, or was moving it out of storage simply a way to suppress symptoms?

No, but implementing Equatable should require some care if we want something more powerful than representation equality. It would be reasonable to expect that the order of the elements in AST.nodes is not significant.

Why? Isn't parsing a deterministic process? Regardless, if it weren't, that care is easy enough to take if we know each node's type and source range; we can just sort them before comparing.

As a general principle, unless there's a really good reason why it cannot be so, all values ought to be Equatable.

[dabrahams]

Note that I think we'll have to accept that two ASTs parsed from the same sources might have different representations if we want to ever support incremental compilation.

I personally don't care whether the representations match as long as they can be compared for equality. Arguably anything containing a Dictionary or Set has the same property, but that doesn't cause a problem for Equatable.

[kyouko-taiga]

Wow, I'm so lost now.

Nothing was identified or solved. The bug just showed up when Lucian increased the memory footprint of the AST. Presumably I did the same when I wrote coreTrait in Storage and moving it out kept the UB's symptoms hidden. My mistake was to not report the strange behaviors I observed at that time.

[kyouko-taiga]

I personally don't care whether the representations match as long as they can be compared for equality. Arguably anything containing a Dictionary or Set has the same property, but that doesn't cause a problem for Equatable.

I am not saying we can't define equality, only that if we want to do it correctly we probably can't just slap : Equatable on the AST declaration, depending on what we'll use this equality for. Thinking ahead about incremental compilation, we'll have to deal with ASTs that don't assign the same IDs to the same nodes.

[dabrahams]

We need to have a more general discussion about what we mean by incremental compilation and what its implications will be for our data structures, c.f. #1163 (comment)