This is now in a reviewable shape, I think.

The second commit might be over-engineered, though. 😅

I've had a look at this and the PRs on top of this. I think it fits the overall linker design reasonably nicely.

Some higher level comments:

• IMO the way we flag for desugaring is not very nice at the moment. I'm wondering if just membermethodsToDesugar: Map[MemberNamespace, Set[MethodName]] inLinkedClass would do the trick.

• I'm wondering if we could doMore DCE wrt case object equality test #2396 in this phase (fold_ === LoadModule(_) to false in case the module is not instantiated, the analyzer could flag for "desugaring" if it sees this shape, which it will need to recognize anyways).

• How doKnowledge accessor utilities #5099 /Refactor: Introduce common caching utilities. #5098 factor into this picture? (I have not looked at them in very much detail).

• The second commit might be over-engineered, though. 😅

  I think there is value in having a better structured configuration value. However, I would not put any validation logic into it (and just "hard-code" that in the checkers, just like it is now).

I'll likely proceed with a more detailed review when I have time, unless you feel comparing this to#5096 needs more higher level discussion.

• IMO the way we flag for desugaring is not very nice at the moment. I'm wondering if just membermethodsToDesugar: Map[MemberNamespace, Set[MethodName]] inLinkedClass would do the trick.

I can try something like that.

• I'm wondering if we could doMore DCE wrt case object equality test #2396 in this phase (fold_ === LoadModule(_) to false in case the module is not instantiated, the analyzer could flag for "desugaring" if it sees this shape, which it will need to recognize anyways).

I don't think we can. The problem is that wedon't see the shape_ === Load module(_) at this stage. We see something likeLoadModule.equals(y). We have to be able toinline theequals before we see the shape. That's not something we can do at the desugaring phase. We need the optimizer to do it.

• How doKnowledge accessor utilities #5099 /Introduce common caching utilities. #5098 factor into this picture? (I have not looked at them in very much detail).

They don't. Those two PRs are unrelated. (I was prompted to look into that because I expected to have to write one more cache with clear after run for the desugaring phase. But it turned out it was so fast that a cache makes it worse, and so there is no relationship with the cache PRs.)

• The second commit might be over-engineered, though. 😅

  I think there is value in having a better structured configuration value. However, I would not put any validation logic into it (and just "hard-code" that in the checkers, just like it is now).

So basically making it a bare enum-like thing with an ordering, and have the checkers know the acceptable ranges? That's what I started with, but found it unwieldy. I can revert to that state to give a comparison.

I'll likely proceed with a more detailed review when I have time, unless you feel comparing this to#5096 needs more higher level discussion.

No I think it's fine. It seems that this variant wins over#5096 overall.

So basically making it a bare enum-like thing with an ordering, and have the checkers know the acceptable ranges? That's what I started with, but found it unwieldy. I can revert to that state to give a comparison.

Maybe consider giving the enum ordering to make the range checks easier to grok. Or even expand the features on top of the checker (val newLambdaAllowed = ...).

IMHO the important part is that the logic of what is allowed where is in the checkers and not the phase enum.

I pushed a different version of theCheckingPhase refactoring. I removed logic fromCheckingPhase; it is now a pure enum. However, sinceClassDefChecker andIRChecker must agree on "which feature is available when", I still kept that logic in a common place, which is nowFeatureSet.FeatureSet isprivate[checked], so it is only an implementation detail of the two checkers. From the checkers' perspective, we use "logical" features now.

This should make things clearer while leaving the responsibility to the checkers, as you mentioned (although factored out inFeatureSet.supportedBy).

• IMO the way we flag for desugaring is not very nice at the moment. I'm wondering if just membermethodsToDesugar: Map[MemberNamespace, Set[MethodName]] inLinkedClass would do the trick.

I can try something like that.

I pushed a commit thast switches to that scheme. I'm not sure I like it better, though. It bothers me that we have fields inLinkedClass andLinkedTopLevelExport that are only useful betweenBaseLinker andDesugarer.

It bothers me that we have fields in LinkedClass and LinkedTopLevelExport that are only useful between BaseLinker and Desugarer.

Yes, I agree, this is not nice in our design. However, I don't think this is anything new:LinkedClass contains a lot of fields only useful for the frontend pipeline (e.g.fieldsRead, all the module dependency information).

Arguably, this is something we should fix. Ironically, one of the easiest way I know is using structural subtyping like in go (producer exposes concrete type, consumer defines an interface of what it needs). Unfortunately, that's not really available to us in Scala.

Other than that, I find the alternative much cleaner :-/ Do you think it makes sense to figure out how to better decouple the fields we have for individual phases?

However, I don't think this is anything new:LinkedClass contains a lot of fields only useful for the frontend pipeline (e.g.fieldsRead, all the module dependency information).

Good point. The new desugaring info does not make the situation qualitatively worse.

This is probably fine as is, then.

We can try to improve that afterwards.

How big is the effort to split this into a separate PR to ease review?

I can do that, but probably not before Wednesday.

I can do that, but probably not before Wednesday.

No worries. I will not have time before that for sure.

Rebased on top of#5120.

Rebased. This is now back to being standalone and re-reviewable.

I went forDesugaringRequirements.

Also I changed how we construct instances ofDesugaringRequirements, in a way that provides a fastisEmpty test. This allowsDesugarer to entirely bypass classes that need no desugaring. It's now running in 3.5 ms on my machine.