Movatterモバイル変換

src/compiler/scala/tools/nsc/typechecker/Namers.scala OutdatedShow resolvedHide resolved

som-snytt force-pushed thefollowup/12159-mixed-java-sealed branch from8751aeb toeeac512Compare

April 1, 2023 09:20

som-snytt changed the title~~Improve exhaustiveness check for mixed java sealed~~Improve exhaustiveness check for mixed java sealed [ci: last-only]

Apr 1, 2023

Copy link

ContributorAuthor

som-snytt commentedApr 1, 2023•
edited
Loading

The updated commit just "registers" Java classes and matches for completion after typer, where (per lrytz) either there is a permits or we look at classes from the same source file.

The simple impl could be improved or complexified further. Or simplified.

som-snytt marked this pull request as draft

April 1, 2023 10:01

som-snytt force-pushed thefollowup/12159-mixed-java-sealed branch fromeeac512 to35c0384Compare

April 2, 2023 04:53

Copy link

Member

lrytz commentedApr 3, 2023

Thanks, great to see that we can make it work!

Why do you think we should delay until after typer? Forcing is what the typer does, I don't see a particular risk of running into cycles here, we don't need to infer anything to complete these class symbols.

For finding symbols defined in a particular compilation unit, I also think we need new global state, there doesn't seem to be anything we can reuse. A question is where to put it, I guess it doesn't matter too much. Here's a draft, what do you think (last commit)?2.13.x...lrytz:scala:pr10348

Copy link

ContributorAuthor

som-snytt commentedApr 3, 2023•
edited
Loading

Yes, that is what I was thinking of (a perRunCaches Map). I thought keeping the logic in one place was simpler, "children we know about" and "other offspring". Oh I see what you mean, we've already seen all the classes for this unit, nothing is gained by waiting.

The other piece I started looking at was emitting the permittedSubclasses attribute for Scala sealed. I'll try to add a commit for that, I was about to imitate some structure for innerClasses.

A final clean-up is also to review the tests for duplication or completeness.

Also, this is a patmat feature, so I like that it looks that way by putting the unwieldy sources map there.

Copy link

ContributorAuthor

som-snytt commentedApr 3, 2023•
edited
Loading

I added your commits and will push after cleaning up my detritus.

Also I haven't touched the java parser to look more like your earlier edit, which I remember preferring.

som-snytt force-pushed thefollowup/12159-mixed-java-sealed branch from35c0384 toa646610Compare

April 3, 2023 10:52

som-snytt commented

src/compiler/scala/tools/nsc/typechecker/Namers.scala

		valpluginsTp= pluginsTypeSig(res, typer, cdef,WildcardType)
		cdef.getAndRemoveAttachment[PermittedSubclasses].foreach { permitted=>
		clazz.updateAttachment[PermittedSubclassSymbols] {
		PermittedSubclassSymbols(permitted.permits.map(typer.typed(_,Mode.NOmode).symbol))

Copy link

ContributorAuthor

som-snyttApr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

what happens to errors on bad permits clause here?

som-snytt commented

src/compiler/scala/tools/nsc/typechecker/Typers.scalaShow resolvedHide resolved

som-snytt commented

src/compiler/scala/tools/nsc/typechecker/Typers.scala OutdatedShow resolvedHide resolved

som-snytt commented

src/partest/scala/tools/partest/nest/AbstractRunner.scalaShow resolvedHide resolved

som-snytt marked this pull request as ready for review

April 3, 2023 11:57

lrytz reviewed

Apr 4, 2023

Copy link

Member

lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We forgot about inner classes...

package p;public sealed interface X {  public default int x() { return 1; }}final class Y implements X { }final class O {  final static class Z implements X { }}

package pclass T {  def n(a: X) = a match { case _: Y => 0 }}

Should warn but doesn't in mixed compilation. Namer eagerly greates symbols for top-level classes. But symbols for inner classes are only created when the outer class completes.

So if there's a sealed Java type without apermits clause, we probably have to recursively force all classes defined in the same compilation unit.

X.java:9: error: anonymous classes must not extend sealed classes    X bar = new X() { };                    ^

😅 OK, at least that

@retronym brought runtime reflection to the table:

scala> import scala.reflect.runtime.{universe => ru}import scala.reflect.runtime.{universe=>ru}scala> ru.typeTag[p.X].tpe.typeSymbol.asClass.knownDirectSubclassesval res0: Set[reflect.runtime.universe.Symbol] = Set()

Also macros / compiler plugins might want to see children, not only the exhaustivity checker. But I think it's fine to defer that.

src/compiler/scala/tools/nsc/transform/patmat/PatternMatching.scalaShow resolvedHide resolved

som-snytt changed the title~~Improve exhaustiveness check for mixed java sealed [ci: last-only]~~Support Java 17sealed in Java sources [ci: last-only]

Apr 24, 2023

som-snytt force-pushed thefollowup/12159-mixed-java-sealed branch 2 times, most recently from2bf8131 to62c5acfCompare

April 25, 2023 00:48

som-snytt changed the title~~Support Java 17sealed in Java sources [ci: last-only]~~Support Java 17sealed in Java sources

Apr 25, 2023

Copy link

ContributorAuthor

som-snytt commentedApr 26, 2023

@lrytz this will be green after squashing. I guess "zucchini", as a green squash.

I'll take another look at the tweak to populating the sealed class hierarchy. It needs tests withnon-sealed descendants.

Populate children of Javasealed hierarchy

a0f546d

Required for checking permitted subclassesand for exhaustiveness of patterns.Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>

lrytz force-pushed thefollowup/12159-mixed-java-sealed branch from9c3e87d toa0f546dCompare

May 1, 2023 09:20

lrytz approved these changes

Copy link

Member

lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🥒 ed

LGTM, thank you!

lrytz merged commitbd9b10a intoscala:2.13.x

lrytz mentioned this pull request

Restore test fixed by recent Java-sealed PR#10386

Merged

som-snytt deleted the followup/12159-mixed-java-sealed branch

May 1, 2023 14:32

SethTisue added the release-notesworth highlighting in next release notes label