-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow experimental language imports in experimental scopes #13396
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
Conversation
An experimental language import is permitted of the enclosing scope is an experimental scopes, or if all statements in that scope are @experimental definitions.
} | ||
|
||
class Test7 { | ||
import scala.language.experimental |
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.
Deleted since we now check experimental imports after typer, and Test7 produces errors at Typer
I would not say that it is an alternative, it is an orthogonal improvement. This makes imports experimental scope aware while #13394 allows the import of This PR and #13394 complement each other. |
@@ -0,0 +1,7 @@ | |||
import language.experimental.erasedDefinitions // error |
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.
To unblock #11721, this should not be an error. See dotty-staging@ea8cb80#diff-0ca71061afa8605808278d2aad85e45877447c09bc11be7c51d3a2e0ad35edc4R1
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.
See pos-curstomArgs/experimental.erased
for the same example that compiles. It's not CanThrow
that triggers the error but other
.
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 tested pos-curstomArgs/experimental.erased
manually with -Yno-experimental
. I assume that will be tested anyway since we start with a stable compiler. Or do we need a separate pos test that asserts -Yno-experimental`?
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.
That is the same behavior as in master.
import language.experimental.erasedDefinitions
import annotation.experimental
@experimental
erased class CanThrow[-E <: Exception]
Compiles with snapshot build without error but fails when we add -Yno-experimental
.
If we cannot compile it with -Yno-experimental
we will not be able to re-bootstrap on a stable compiler.
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 tested it manually with -Yno-experimental
and verified that it compiles
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 would be good to port this test https://github.com/lampepfl/dotty/pull/13394/files#diff-0ca71061afa8605808278d2aad85e45877447c09bc11be7c51d3a2e0ad35edc4
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.
Ah I see what goes on. The generated package object is not experimental.
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.
The error message of is also unhelpful. If a user does this change they will have no idea why it started failing.
import language.experimental.erasedDefinitions // error
import annotation.experimental
@experimental
erased class CanThrow[-E <: Exception]
+ object Foo
The scope of CanThrow
is still experimental. But now it it says Experimental features may only be used with a nightly or snapshot version of the compiler
which has not changed.
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 now disregard synthetic definitions in the check. That also takes care of compiler-generated companion objects.
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 https://github.com/lampepfl/dotty/pull/13396/files#diff-53335a51b3630ff685db4a7f8b0e67d06bc74d3fb9c1d7e642a5e864ff240187R738 is the wrong way to generate the error message.
Yes, and isn't that a problem by itself? EDIT: No, since we test manually afterwards at use sites. But are we sure we found all
Not true. I believe I have been quite precise in my spec and implementation and it does allow |
The main difference where #13394 allows more:
Where #13396 allows more:
|
It seems this design does not allow us to add experimental erased methods. Here we assume that import language.experimental.erasedDefinitions // error
import annotation.experimental
object Foo:
def foo = 1
@experimental
erased def bar = 2 |
The extra scope rules should be added to |
We will probably need to define new erased definitions in the future before we stabilize erased. |
Yes, maybe. I still dislike #13394 since it mixes two things that should stay separate. But maybe we need it. On the other hand, maybe it's better to spend our energy on stabilizing |
# Conflicts: # compiler/src/dotty/tools/dotc/typer/Checking.scala
Also noticed that rule (1) is not properly tested. I added the tests in #13417. |
Actually all tests in this PR pass on master. |
Replaced with #13417 |
An experimental language import is permitted if the enclosing
scope is an experimental scope, or if all non-synthetic statements in that
scope are @experimental definitions.
Fixes #13392
Alternative to #13394