Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cut the Gordian Knot: Don't widen unions to transparent #15642

Merged
merged 11 commits into from
Nov 9, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 10, 2022

The idea is that some unions usually make more sense than others. For instance,
if Apply and Ident are case classes that extend Tree, it makes sense to
widen Apply | Ident to Tree. But it makes less sense to widen String | Int
to Matchable.

Making sense means: (1) Matches our intuitive understanding, and (2) choosing not to
widen would usually not cause errors.

To explain (2): In the Tree case it might well be that we define a given for Inv[Tree] for
invariant class Inv, and then we would not find that given for Inv[Apply | Ident].
But it's much less likely that we are looking for a given of type Inv[Any].

This commit does two things:

  • add logic not to widen a union if the result is a product of only transparent traits or classes.
  • treat Any, AnyVal, Object, and Matchable as transparent.

@odersky odersky marked this pull request as draft July 10, 2022 18:21
@odersky
Copy link
Contributor Author

odersky commented Jul 10, 2022

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15642/ to see the changes.

Benchmarks is based on merging with main (79d9a6f)

@sjrd
Copy link
Member

sjrd commented Jul 11, 2022

  • treat Any, AnyVal, Object, and Matchable as transparent.

If that makes sense, then it probably makes sense to also treat js.Any and js.Object as transparent, when they are there:
https://github.com/lampepfl/dotty/blob/1fb83bda632c0ed059844273e0789daeac24af8e/compiler/src/dotty/tools/backend/sjs/JSDefinitions.scala#L45-L48


Unrelated to the above: I expect this change to have a significant impact on source compatibility. Although we never promise source compatibility, perhaps this one is strong enough that it would warrant a minor release?

@odersky
Copy link
Contributor Author

odersky commented Jul 11, 2022

@sjrd I agree it would need a minor release. It might need a SIP as well, even though so far we have not talked about type inference in the SIP committee, despite many changes and improvements that happened in Scala 2 and 3.

That said, I am positively surprised how little code broke. All the tests that failed were simply testing for type inference predictions that are now improved. Nothing else in our code base or in the community build broke. I would not have expected that when I set out to try this change.

@odersky odersky added the needs-minor-release This PR cannot be merged until the next minor release label Jul 11, 2022
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM (commetting to avoid it being merged)

@odersky odersky marked this pull request as ready for review July 11, 2022 15:34
The idea is that some unions usually make more sense than others. For instance,
if `Apply` and `Ident` are case classes that extend `Tree`, it makes sense to
widen `Apply | Ident` to `Tree`. But it makes less sense to widen `String | Int`
to `Matchable`.

Making sense means: (1) Matches our intuitive understanding, and (2) choosing not to
widen would usually not cause errors.

To explain (2): In the `Tree` case it might well be that we define an implicits on `Inv[Tree]` for
invariant class `Inv`, and then we would not find the implicit for `Inv[Apply | Ident]`.
But it's much less likely that we are looking for an implicit of type `Inv[Any]`.

This commit does two things:

 - add logic not to widen a union if the result is a product of only transparent traits or classes.
 - treat `Any`, `AnyVal`, `Object`, and `Matchable` as transparent.
 - include js.Any and js.Object
 - include others ...Ops and Is... classes from collections
 - Change the implementation so that we don't have to load
   traits or classes that are assumed transparent.
They give test failures, and I don't know enough about the js class hierarchy to
be able to fix them with confidence.
@dwijnand
Copy link
Member

dwijnand commented Nov 8, 2022

@odersky happy to merge this now?

@odersky
Copy link
Contributor Author

odersky commented Nov 8, 2022

I am OK with merging. I'll give this another 24 hours in case someone wants to object.

@smarter
Copy link
Member

smarter commented Nov 8, 2022

I'm a bit wary of trying to solve our union inference issues by adding more special cases, in particular it might have the unintended consequence of encouraging people to add transparent in their class hierarchies to avoid widening. In any case, this change of behavior requires an update of the documentation since both https://github.com/lampepfl/dotty/blob/main/docs/_docs/reference/new-types/union-types.md an https://github.com/lampepfl/dotty/blob/main/docs/_docs/reference/new-types/union-types-spec.md refer to the current widening logic.

 - Introduce hard and soft unions
 - Explain how transparency of base traits influences type inference
 - Drop outdated note on possible future changes
@odersky
Copy link
Contributor Author

odersky commented Nov 9, 2022

@smarter I updated the docs on union types. Do you want to take a look?

@odersky odersky merged commit 1276ba5 into scala:main Nov 9, 2022
@odersky odersky deleted the change-widen-union branch November 9, 2022 20:08
@som-snytt
Copy link
Contributor

add transparent in their class hierarchies to avoid widening.

add transparent until it compiles. Is that not the FAQ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants