-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refine FlexibleType nullification rules #23938
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
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.
Otherwise LGTM
case FlexibleType(_, parent2a) if ctx.flexibleTypes => | ||
FlexibleType(derivedRefinedType(tp, parent2a, refinedInfo2)) | ||
case OrNull(parent2a) => | ||
OrNull(derivedRefinedType(tp, parent2a, refinedInfo2)) | ||
case _ => | ||
derivedRefinedType(tp, parent2, refinedInfo2) |
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.
Would you prefer this approach? @olhotak
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.
We can try it.
I'm a bit nervous about flexifying the parent type and then undoing that (what if it was previously flexified already?), but we can try it.
A simpler approach would be to always flexify the refined type at the top level but then to not flexify the parent type. That may unnecessarily flexify a refined type in cases when the parent is one of the types that we don't flexify (such as a primitive type), but it seems unlikely that people would want to refine such types.
I think it doesn't make sense to flexify the parent type because it doesn't make sense to refine null. You wouldn't say (Foo|null) { def bar(): Int }
. Instead, you could say (Foo { def bar(): Int }) | Null
.
I have run the compilation tests locally. The tests below fail because of the current awkward stage of stdlib.
|
4b00c5c
to
299a72a
Compare
It seems to be fixing 13 projects in the OpenCB, however some of reported projects are now failing with different errors requring wrong number of arguments - I have not seen errors like that before.
Error: 166 | check(genTuple(Gen.int, Gen.int.filter(_ != 0))) { (num, div) =>
Error: | ^
Error: | Wrong number of parameters, expected: 1
Error: 167 | val eff = zArrow(num).provide(ZLayer.succeed(div))
Error: 168 | assertZIO(eff)(equalTo(num / div))
Error: |
Error: -- [E086] Syntax Error: /build/repo/ordset/src/ordset/util/ShowUtil.scala:62:67
Error: 62 | val indentedLines = lines.zipWithIndex.map { (line, index) =>
Error: | ^
Error: | Wrong number of parameters, expected: 1
Error: 63 | if (index == 0) line else fieldIndent + line
Error: |
Error: | longer explanation available when compiling with `-explain`
Error: -- [E046] Cyclic Error: /build/repo/facsimile-util/src/test/scala/org/facsim/util/test/MemoizeTest.scala:224:17
Error: 224 | forAll { (il: List[Int]) =>
Error: | ^
Error: | Cyclic reference involving trait BitSet
Error: |
Error: | Run with -explain-cyclic for more details.
Error: |
Error: -- [E007] Type Mismatch Error: /build/repo/univeq/shared/src/main/scala-3/japgolly/univeq/internal/Derivation.scala:78:61
Error: 78 | log(" - Checking: " + Type.show(using fUnit.asType))
Error: | ^^^^^^^^^^^^
Error: | Found: (scala.quoted.Type[? <: AnyKind])?
Error: | Required: scala.quoted.Type[T]
Error: |
Error: | where: T is a type variable with constraint <: AnyKind
Error: |
Error: | longer explanation available when compiling with `-explain` |
I would guess that the errors about wrong number of arguments are due to some failure of auto-tupling probably getting confused by flexible types. |
Based on #23911
Fixes #23908
Could fix #23933, #23935, #23936, and #23937 (I tested locally)?
DO NOT MERGE: the explicit nulls tests are currently broken
cc @olhotak @HarrisL2