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

Add message parameter to @experimental annotation #19935

Conversation

nicolasstucki
Copy link
Contributor

Once we bootstrap this addition, we will be able to enhance the messages of @experimental definitions in the library, the annotations added -experimental and the ones we plan to add for language features in #19807.

@nicolasstucki nicolasstucki marked this pull request as ready for review March 13, 2024 12:13
@nicolasstucki nicolasstucki self-assigned this Mar 13, 2024
@nicolasstucki nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Mar 13, 2024
@nicolasstucki nicolasstucki force-pushed the add-message-to-experimental-annotation branch 2 times, most recently from 4972375 to e99f5e5 Compare April 2, 2024 09:54
@Gedochao Gedochao requested a review from sjrd April 3, 2024 14:08
@Gedochao
Copy link
Contributor

Gedochao commented Apr 3, 2024

It has been decided to be included in the 3.5.0 release.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Some small comments.

@@ -131,12 +131,7 @@ object Feature:

def checkExperimentalFeature(which: String, srcPos: SrcPos, note: => String = "")(using Context) =
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the note parameter is never used anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used here

report.error(experimentalUseSite(which) + note, srcPos)
                                          ^^^^

and comes from

// Checking.scala
Feature.checkExperimentalFeature("features", imp.srcPos,
              s"\n\nNote: the scope enclosing the import is not considered experimental because it contains the\nnon-experimental $stable")

Comment on lines 6 to 7
import Annotations.Annotation
import Constants.Constant
Copy link
Member

Choose a reason for hiding this comment

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

Spurious changes.

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

then
sym.addAnnotation(Annotation(defn.ExperimentalAnnot, sym.span))
if sym.is(Module) then
sym.companionClass.getAnnotation(defn.ExperimentalAnnot).foreach(sym.addAnnotation)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is safe to take an annotation tree and put it in two different places in the AST. Consider creating an accurate copy instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some helper methods in Annotations.ExperimentalAnnotation.

@sjrd sjrd removed their assignment Apr 3, 2024
@nicolasstucki nicolasstucki force-pushed the add-message-to-experimental-annotation branch from e99f5e5 to dbdfcff Compare April 5, 2024 09:20
@nicolasstucki nicolasstucki merged commit 133e709 into scala:main Apr 8, 2024
19 checks passed
@nicolasstucki nicolasstucki deleted the add-message-to-experimental-annotation branch April 8, 2024 15:48
@noti0na1
Copy link
Member

noti0na1 commented Apr 8, 2024

I have an experimental function added to Predef in #18112, this PR causes cyclic references between experimental.scala, Predef.scala, etc.

@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
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