Check flags for `newMethod`, `newVal` and `newBind` #16565

nicolasstucki · 2022-12-21T10:44:23Z

No description provided.

smarter · 2023-01-03T17:04:24Z

library/src/scala/quoted/Quotes.scala

+       *  A trait that has only abstract methods as members
+       *  and therefore can be represented by a Java interface.


That description looks incorrect abstract override is a specific concept in Scala that is unrelated to Java interfaces: https://www.scala-lang.org/files/archive/spec/2.13/05-classes-and-objects.html#abstract-override

I aligned the doc with https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/dotc/core/Flags.scala#L282-L288

This part of the documentation comment is for PureInterface, not for AbsOverride

I updated the docs.

I noticed that AbsOverride is pickled as Abstract | Override and then unpickled as AbsOverride. Maybe we should not have this flag in the reflection interface and handle AbsOverride as if both Abstract and Override.

I noticed that AbsOverride is pickled as Abstract | Override and then unpickled as AbsOverride. Maybe we should not have this flag in the reflection interface and handle AbsOverride as if both Abstract and Override.

I'm not sure diverging from the compiler implementation is a good idea here. In the specification, abstract override is a separate concept from abstract and override, it just reuses existing keywords. The tasty serialization format also reuses the flags bits, but that's just an optimization.

smarter · 2023-01-03T17:06:18Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

      def newMethod(owner: Symbol, name: String, tpe: TypeRepr, flags: Flags, privateWithin: Symbol): Symbol =
+        import Flags.*
+        // TODO: missing AbsOverride
+        checkValidFlags(flags.toTermFlags, Private | Protected | Override | Deferred | Final | Method | Implicit | Given | Local | JavaStatic) // Synthetic | ExtensionMethod | Exported | Erased | Infix | Invisible


It would be nicer to create a val for each of these long lists of flags. Also couldn't this be checked in -Ycheck?

The idea of the -Xcheck-macros flag is to help users find the location in their implementation where the bug started. Additionally, waiting until -Ycheck can result in other derived failures or crashes before we reach the checks.

smarter · 2023-04-28T11:31:52Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+      private[QuotesImpl] def validMethodFlags: Flags = Private | Protected | Override | Deferred | Final | Method | Implicit | Given | Local | JavaStatic | AbsOverride // Synthetic | ExtensionMethod | Exported | Erased | Infix | Invisible
+      private[QuotesImpl] def validValFlags: Flags = Private | Protected | Override | Deferred | Final | Param | Implicit | Lazy | Mutable | Local | ParamAccessor | Module | Package | Case | CaseAccessor | Given | Enum | JavaStatic | AbsOverride //  Synthetic | Erased | Invisible
+      private[QuotesImpl] def validBindFlags: Flags = Case // | Implicit | Given | Erased


What are the commented-out flags meant to represent? A comment might be helpful.

Added comments

smarter · 2023-04-28T11:35:30Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

      def Trait: Flags = dotc.core.Flags.Trait
      def Transparent: Flags = dotc.core.Flags.Transparent
+
+      private[QuotesImpl] def validMethodFlags: Flags = Private | Protected | Override | Deferred | Final | Method | Implicit | Given | Local | JavaStatic | AbsOverride // Synthetic | ExtensionMethod | Exported | Erased | Infix | Invisible


It looks like the list here is duplicated in the documentation of newMethod, we should add a comment here to remember to update the list in the documentation.

I don't think users can create JavaStatic methods.

I don't think users can create JavaStatic methods.

We have one use case of this with the main macro annotation in tests/run-macros/annot-macro-main.

smarter · 2023-04-28T11:36:05Z

library/src/scala/quoted/Quotes.scala

      *  @param parent The owner of the binding
      *  @param name The name of the binding
-      *  @param flags extra flags to with which the symbol should be constructed
+      *  @param flags extra flags to with which the symbol should be constructed. `Case` flag will be added. Can be `Case`


If Case is always added and Case is the only valid flag, it sounds like this parameter is not actually necessary?

In the future, we might have more flags (see validBindFlags).

We could add an overload of newBind that does not require the flags.

library/src/scala/quoted/Quotes.scala

Co-authored-by: Guillaume Martres <smarter@ubuntu.com>

nicolasstucki force-pushed the macro-check-valid-flags branch from e4ddc97 to b60afdc Compare December 21, 2022 12:37

nicolasstucki marked this pull request as ready for review December 21, 2022 15:02

nicolasstucki requested a review from smarter December 21, 2022 15:02

nicolasstucki assigned smarter Dec 21, 2022

nicolasstucki force-pushed the macro-check-valid-flags branch from b60afdc to 164e407 Compare December 21, 2022 15:15

smarter requested changes Jan 3, 2023

View reviewed changes

smarter assigned nicolasstucki and unassigned smarter Jan 3, 2023

nicolasstucki force-pushed the macro-check-valid-flags branch 2 times, most recently from 9fdfb4a to 09cf574 Compare January 9, 2023 12:07

nicolasstucki assigned smarter and unassigned nicolasstucki Jan 10, 2023

nicolasstucki requested a review from smarter January 10, 2023 08:54

nicolasstucki force-pushed the macro-check-valid-flags branch from 959064f to a85d0ea Compare February 7, 2023 12:47

nicolasstucki force-pushed the macro-check-valid-flags branch from a85d0ea to a4db0c1 Compare March 9, 2023 10:16

nicolasstucki force-pushed the macro-check-valid-flags branch from a4db0c1 to 91a6b57 Compare April 25, 2023 13:24

smarter requested changes Apr 28, 2023

View reviewed changes

smarter assigned nicolasstucki and unassigned smarter Apr 28, 2023

Add reflect Flags.AbsOverride

3a20cd4

nicolasstucki force-pushed the macro-check-valid-flags branch from d3a9810 to d04a7e2 Compare May 2, 2023 09:13

Check flags for method, val and bind symbols

3c3c0fb

Co-authored-by: Guillaume Martres <smarter@ubuntu.com>

nicolasstucki force-pushed the macro-check-valid-flags branch from d04a7e2 to 3c3c0fb Compare May 2, 2023 09:20

nicolasstucki assigned smarter and unassigned nicolasstucki May 3, 2023

nicolasstucki requested a review from smarter May 3, 2023 13:05

smarter approved these changes May 3, 2023

View reviewed changes

smarter merged commit b8d2966 into scala:main May 3, 2023

smarter deleted the macro-check-valid-flags branch May 3, 2023 13:11

pweisenburger mentioned this pull request Jun 4, 2023

Checking flags for new symbols using Quotes API is too restrictive #17764

Closed

Kordyjan added this to the 3.3.1 milestone Aug 2, 2023

		* A trait that has only abstract methods as members
		* and therefore can be represented by a Java interface.

Check flags for newMethod, newVal and newBind #16565

Check flags for newMethod, newVal and newBind #16565

Uh oh!

Conversation

nicolasstucki commented Dec 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Check flags for `newMethod`, `newVal` and `newBind` #16565

Check flags for `newMethod`, `newVal` and `newBind` #16565