Skip to content

Refactor FlexibleType to use TypeBounds instead of case class#26245

Draft
HarrisL2 wants to merge 18 commits into
scala:mainfrom
HarrisL2:type-apply-flexible-type
Draft

Refactor FlexibleType to use TypeBounds instead of case class#26245
HarrisL2 wants to merge 18 commits into
scala:mainfrom
HarrisL2:type-apply-flexible-type

Conversation

@HarrisL2

@HarrisL2 HarrisL2 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Refactors FlexibleTypes to have TypeBounds as underlying representation, which can help with macro-related code avoid match errors.

The key change is in Definitions.scala

How much have you relied on LLM-based tools in this contribution?

Minimally, for some refactoring

How was the solution tested?

Covered by existing tests (this is a refactoring)

@sjrd

sjrd commented Jun 5, 2026

Copy link
Copy Markdown
Member

I don't think this is going in the right direction. Superficially, this may allow some macro test suites to pass in the short term. But the logic will probably be broken anyway. And in the long term, this will just force macros to handle flexible type specially anyway, like the compiler. IMO we should stick with the correct design with real FlexibleType, and adjust the few macros that have chosen to pattern match on type shapes. That is fragile, and will eventually fail for other reasons.

@HarrisL2 HarrisL2 force-pushed the type-apply-flexible-type branch from dbaa86b to 5625b53 Compare June 9, 2026 19:47
@odersky

odersky commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

I disagree with @sjrd. A FlexibleType should behave exactly like a type constructor Flex[T] >: T | Null <: T. It does have some interactions with null-checking, but the above approximation is perfectly fine for macros that don't want to go into the details of null checking.

Also, FlexibleType really looks oddly specific given how general all the other forms of Type are. We only have one other type constructor that is speficic to Java interop: JavaArrayType. But that one only comes to live after erasure, so most of Typer is unaffected. But contrast the FromJavaObjectType has a role similar to FlexibleType, and that one is a predefined symbol, not a new form of type.

@olhotak

olhotak commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

One technical difference that we need to be careful about is that underlying is defined differently for flexible types than for applied types. For an applied type, underlying returns the type constructor, while for a flexible type, it returns the upper bound. Code that interprets a type by looking at its upper bound should "just work" for an applied type representation of flexible types, but we need to be careful about code that interprets a type by calling underlying directly.

@olhotak

olhotak commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

If we represent flexible types internally in the compiler using applied types, a remaining open question is how we want to expose them on the outside, specifically (1) in TASTy and (2) in the Quotes reflection interface.

(1) In TASTy, there is currently an explicit representation of a flexible type. Currently, this PR keeps that representation, i.e., it detects an internal applied type that represents a flexible type and expresses it as an explicit FLEXIBLEtype in TASTy.

(2) The Quotes reflection interface is sructured as a set of pattern match extractors. Currently in this PR, a flexible type represented by an applied type is matched by both the applied type extractor and by the specific flexible type extractor.

@odersky

odersky commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

One technical difference that we need to be careful about is that underlying is defined differently for flexible types than for applied types. For an applied type, underlying returns the type constructor, while for a flexible type, it returns the upper bound. Code that interprets a type by looking at its upper bound should "just work" for an applied type representation of flexible types, but we need to be careful about code that interprets a type by calling underlying directly.

Note that there's also superType that behaves the same in both cases (picks the upper bound).

@olhotak

olhotak commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

@odersky

odersky commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

(1) In TASTy, there is currently an explicit representation of a flexible type. Currently, this PR keeps that representation, i.e., it detects an internal applied type that represents a flexible type and expresses it as an explicit FLEXIBLEtype in TASTy.

I think it would be better to leave the applied type representation. That means FLEXIBLEtype would need to be kept around only for backwards compatibility. That would make the format simpler (we could relegate discussion of FLEXIBLEtype to a separate section on legacy tags).

(2) The Quotes reflection interface is sructured as a set of pattern match extractors. Currently in this PR, a flexible type represented by an applied type is matched by both the applied type extractor and by the specific flexible type extractor.

I think that's fine.

Here are the results of the Open Community Build on this branch:

It would be good to do a more high level analysis of that. What are the root failures in either system?

@sjrd

sjrd commented Jun 11, 2026

Copy link
Copy Markdown
Member

I disagree with @sjrd. A FlexibleType should behave exactly like a type constructor Flex[T] >: T | Null <: T. It does have some interactions with null-checking, but the above approximation is perfectly fine for macros that don't want to go into the details of null checking.

The content of this PR suggests that this is not the case. There is exactly 0 spot where handling FlexibleType was removed. This suggests that there is no place in the code where a FlexibleType behaves like such a type constructor.

Quite the opposite in fact. In several places, the case FlexibleType had to be moved up to come before the case AppliedType, to prevent the behavior of AppliedType.

It would be much more believable if this PR were removing more code than it's adding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants