Replace symbol traversal with tree traversal when finding top level experimentals #21827
Conversation
| else if isNonExperimentalTopLevelDefinition(sym) then | ||
| sym :: Nil | ||
| else Nil | ||
| def nonExperimentalTopLevelDefs(): Iterator[Symbol] = |
There was a problem hiding this comment.
I see this isn't new, but I would prefer to return the List here instead of an Iterator, because Iterator is too easy to misuse (e.g. printing its content to debug it will break it), and we can just directly iterate on the List anyway.
| case defdef: tpd.DefDef => addIfExperimental(defdef.symbol) | ||
| case valdef: tpd.ValDef => addIfExperimental(valdef.symbol) |
There was a problem hiding this comment.
Can be:
| case defdef: tpd.DefDef => addIfExperimental(defdef.symbol) | |
| case valdef: tpd.ValDef => addIfExperimental(valdef.symbol) | |
| case mdef: tpd.ValOrDefDef => addIfExperimental(mdef.symbol) |
| case defdef: tpd.DefDef => addIfExperimental(defdef.symbol) | ||
| case valdef: tpd.ValDef => addIfExperimental(valdef.symbol) | ||
| case typeDef @ tpd.TypeDef(_, temp: Template) if typeDef.symbol.isPackageObject => | ||
| super.apply(x, temp.body) |
There was a problem hiding this comment.
Why super.apply? Shouldn't this be just apply to pick up top-level defs?
| case valdef: tpd.ValDef => addIfExperimental(valdef.symbol) | ||
| case typeDef @ tpd.TypeDef(_, temp: Template) if typeDef.symbol.isPackageObject => | ||
| super.apply(x, temp.body) | ||
| case typeDef @ tpd.TypeDef(_, Template(_, _, _, _)) => addIfExperimental(typeDef.symbol) |
There was a problem hiding this comment.
What about a TypeDef which does not have a Template (type alias / abstract type) ? They should also be marked experimental.
I think that there should be a case after the two TypeDef(_, Template(...)) cases like:
case mdef: tpd.MemberfDef => addIfExperimental(mdef.symbol)and then you can remove the Val/DefDef cases too.
|
Since I'm not sure we test this, it'd be good to pick a random project and check that if we add the -experimental flag, we generate the same tasty content before and after this change, to make sure we're not missing anything. |
dwijnand
changed the title
|
Thank you for the review and suggestions! I have applied review comments and prepared and run a tasty output comparison script between the output using the previous implementation and a new one. I've run it on the output of |
|
@smarter Just realized I forgot to tag you in the message above. Let me know if you would like me to test this PR somewhere else too, or if we are good to go |
There was a problem hiding this comment.
for package objects a ValDef with that symbol can be produced which in the previous implementation would not be annotated, and in the new one would, so I removed that (with the additional case with ValDef if isPackageObject)
Thanks for checking this. It sounds like this was actually a bug in the old implementation and the new behavior is better? So I think we don't need the additional case.
| case tpd.PackageDef(_, contents) => apply(x, contents) | ||
| case typeDef @ tpd.TypeDef(_, temp: Template) if typeDef.symbol.isPackageObject => | ||
| apply(x, temp.body) | ||
| case vdef: tpd.ValDef if vdef.symbol.isPackageObject => x // ValDef(new PackageObject) |
There was a problem hiding this comment.
@smarter Just to confirm - we want to annotate those ValDefs containing package objects then? I'm hesitant since I don't think that is possible to do by annotating by hand with @experimental (it seems we can't annotate package objects at all), but on the other hand this is indeed a top-level def, which is why I'm asking
There was a problem hiding this comment.
You can annotate a packable object defined as object `package` but for those generated for top-level defs there isn't a way to do it indeed. For those it might not matter since you cannot extend them, but it seems more uniform to treat everything the same way and therefore to also annotate them here.
There was a problem hiding this comment.
Sure! Thank you for the review!
…xperimentals (scala#21827) Aims to fix stale symbol errors caused by the symbol traversal after suspending by a macro
…xperimentals (scala#21827) Aims to fix stale symbol errors caused by the symbol traversal after suspending by a macro
Fixes #21802, as suggested by #21802 (comment)
As an aside, in #20409 (which included the changes that are being addressed here) a sbt-scripted test for a crash was added - however I was not able to make that error reproduce, even when hardcoding older scala version there (3.5.0-RC1-bin-20240515-177b489-NIGHTLY). I was able to reproduce the error in a separate sbt build with the same scala version (so there is probably some difference in how things are loaded between a sbt-scripted test and a regular sbt build). No idea how that could be fixed, but I did test those now changes here in that separate repository to confirm there is no regression for that test.