-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Apply flexible types to files compiled without explicit nulls #23386
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
base: main
Are you sure you want to change the base?
Conversation
Test performance please |
a04a482
to
77c3c0c
Compare
[test_scala2_library_tasty] |
test performance please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a commit that doesn't change anything real to force the CI to re-run. (Perhaps change the newly-introduced double newline in TreeUnpickler.)
Otherwise LGTM, but also wait for approval from @noti0na1 .
77c3c0c
to
a584b72
Compare
a584b72
to
d9f49d1
Compare
4a1c1db
to
a2dfe25
Compare
This is ready for re-review. |
I was on vocation in last few weeks. I will review it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some questions about the specific cases.
case tp: AnnotatedType => mapOver(tp) | ||
case tp: OrType => mapOver(tp) | ||
case tp: MatchType => mapOver(tp) | ||
case tp: RefinedType => nullify(mapOver(tp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For OrType
and RefinedType
, I think we should handle them like AndType
: only nullify the outside, and set outermostLevelAlreadyNullable
to true
for each branch and underlying. Another question is, should we also apply the nullification to the refining part or leave it? For example, A { val a: B }
.
case tp: ExprType => mapOver(tp) | ||
case tp: AnnotatedType => mapOver(tp) | ||
case tp: OrType => mapOver(tp) | ||
case tp: MatchType => mapOver(tp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think MatchType
can be tricky.
- We should not add OrNull to the type arguments, because that may change the result of matching; maybe the matching result will not change due to flexible type. Let's test this.
- Not sure whether we should nullify the outside as well, since we don't know the result type is a reference type or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is safer to not nullify match types at all.
A fixed version of #22473
This PR wraps the types of symbols from files compiled without explicit nulls in flexible types.
This allows for interop between multiple files in cases where
compiled without explicit nulls can still be used in
whereas the argument would have been a strictly non-null String, because of the flexible type, the function call is now permitted.
Some explicit nulls neg tests have been excluded under the scala2 library tasty tests due to technical reasons, for example:
val xl = s.length
will not give an error. This is because the .length function is from the WrappedString.scala library. After desugaring, it will becomewrapString(s).length
. Since the scala2 library is compiled without explicit nulls, the types are flexified.wrapString
will have argument type FlexibleType(String) and return type FlexibleType(String), therefore it will not give an error. Hence we need to exclude it from the tasty tests.[test_scala2_library_tasty]