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

Enable experimental mode when experimental feature is imported #19807

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Feb 28, 2024

Make it easier to use experimental language features out of the box. We already have -experimental that enables experimental mode globally by adding the @experimental annotation to all top-level definitions. The complication was that this flag was necessary to use experimental language features. It was not always easy to set (for example in the REPL) and it is all or nothing. Users can also annotate explicitly all top-level definitions that use language features with @experimental.

Now, the import of an experimental language feature will automatically enable experimental mode. If the import is at the top-level, all definitions in the file are experimental.

We also remove the special case that makes all code generated with a nightly or snapshot compiler compile as if in experimental mode. This implies that now the behavior of nightlies is identical to the behavior of release versions. The -Yno-experimental becomes a no-op and will be removed in a later PR.

[test_scala2_library_tasty]

@nicolasstucki nicolasstucki self-assigned this Feb 28, 2024
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch 18 times, most recently from 08f87c3 to 175cb7d Compare March 6, 2024 11:00
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch 5 times, most recently from 3c8108a to 9aa0ce6 Compare March 8, 2024 09:15
@nicolasstucki nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Mar 8, 2024
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch 5 times, most recently from eb477fd to 1e62dca Compare March 12, 2024 07:54
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch 2 times, most recently from 25970ae to 9973893 Compare April 17, 2024 07:38
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch 2 times, most recently from 38e175e to ac64505 Compare April 18, 2024 08:13
@nicolasstucki nicolasstucki removed their assignment Apr 18, 2024
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch 2 times, most recently from 2a0479b to 5c3c5cf Compare April 22, 2024 13:23
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch from 5c3c5cf to 6ecf7cd Compare April 25, 2024 14:41
The `@experimental` flag is added to top-level definitions in the package
where the language feature is imported.
Move similar logic from PostTyper into `checkAndAdaptExperimentalImports`.
Also make the message of `@experimental` more precise for experimental
language settings.
@nicolasstucki nicolasstucki force-pushed the enable-experimental-on-package-level-language-imports branch from 32701e1 to 32f5a17 Compare April 30, 2024 09:27
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

compiler/src/dotty/tools/dotc/typer/Checking.scala Outdated Show resolved Hide resolved
!sym.isExperimental
&& sym.source == ctx.compilationUnit.source
&& !sym.isConstructor // not constructor of package object
&& !sym.is(Package) && !sym.isPackageObject && !sym.name.endsWith(str.TOPLEVEL_SUFFIX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the TOPLEVEL_SUFFIX test we can do sym.name.isPackageObjectName. But why do it at all, since we already have an isPackageObject test. Is that not redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the redundant test

.filter(sym => sym.isClass && (sym.is(Package) || sym.isPackageObject))
.flatMap(nonExperimentalTopLevelDefs)

packageMembers ++ packageObjectMembers
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider combining the two definitions into:

pack.info.decls.toList.iterator.flatMap: sym =>
  if sym.isClass && (sym.isPackage or sym.isPackageObject) then 
     nonExperimentalTopLevelDefs(sym)
  else if isNonExperimentalTopLevelDefinition(sym) then sym :: Nil
  else Nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

LGTM!

@odersky odersky enabled auto-merge April 30, 2024 14:47
@odersky odersky merged commit 3920665 into scala:main Apr 30, 2024
18 checks passed
@odersky odersky deleted the enable-experimental-on-package-level-language-imports branch April 30, 2024 18:02
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
smarter added a commit to dotty-staging/dotty that referenced this pull request May 14, 2024
In scala#19807, the behavior of `-experimental`
was changed to mark all top-level definitions as experimental. To do so, the
implementation traverses the whole package and checks every symbol to see if it
should be transformed or not. The problem was that the first check we do is
`sym.isExperimental` which ends up forcing the symbol.

Besides being a performance issue, this could also lead to a crash if the
current package is the empty package, because we could end up forcing the magic
`module-info.class` that Java modules place there. For some reason, this appear
to only happen when building with sbt, hence the additional scripted test.

This PR fixes this issue by reordering the checks (and adding a preliminary
`isDefinedInCurrentRun` check for good measure). We should also investigate
whether we can avoid creating a symbol for `module-info.class`, but this PR is
intentionally minimal so we can backport it to 3.5.0-RC2 without risks.
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.

5 participants