Add attributes section to TASTy and use it for Stdlib TASTy #18599

nicolasstucki · 2023-09-26T07:48:08Z

The Attributes section contains a list Attribute tags.

At this point, the only Scala 2 TASTy is the one from the standard library. To know that a TASTy contains the definitions of the standard library, we add the SCALA2STANDARDLIBRARYattr attribute to the TASTy file. We mark all unpickled classes from those TASTy files with Scala2x | Scala2Tasty.

Attributes will also be used to mark files that were compiled with explicit nulls using the EXPLICITNULLSattr attribute.

nicolasstucki · 2023-09-29T09:22:58Z

Only the fact that we added a Attributes section to TASTy is important for the release notes.

bishabosha · 2023-10-17T07:22:34Z

will be useful as well to add a JavaSourcefile attribute or equivalent for pipelining purposes

sjrd · 2023-10-25T14:13:42Z

compiler/src/dotty/tools/dotc/core/tasty/AttributeUnpickler.scala

+    val attributesBuilder = List.newBuilder[String]
+    while (!isAtEnd) {
+      val length = readNat()
+      if (length > 0) {


This test probably should not be there. It would most likely be invalid to have a zero length, but if it is not, it means there is actually an empty string attribute.

I will specify and validate that attributes are non-empty strings.

compiler/src/dotty/tools/dotc/core/tasty/AttributePickler.scala

compiler/src/dotty/tools/dotc/core/tasty/AttributeUnpickler.scala

sjrd · 2023-10-25T14:20:49Z

tasty/src/dotty/tools/tasty/TastyFormat.scala

@@ -267,6 +267,9 @@ Standard Section: "Comments" Comment*
 ```none
  Comment       = Length Bytes LongInt      // Raw comment's bytes encoded as UTF-8, followed by the comment's coordinates.
 ```
+
+Standard Section: "Attributes" UTF8*


We should specify here what are the possible attributes. They have semantic meaning, not just a collection of strings, so they should be specified.

I extended the definition of Attributes

sjrd · 2023-10-25T14:21:31Z

It would be good to have a second reviewer here. Adding a TASTy section is a pretty big deal.

compiler/src/dotty/tools/dotc/core/tasty/AttributeUnpickler.scala

nicolasstucki · 2023-10-26T11:41:58Z

compiler/src/dotty/tools/dotc/transform/ExtensionMethods.scala

@@ -77,7 +77,7 @@ class ExtensionMethods extends MiniPhase with DenotTransformer with FullParamete

          // Create extension methods, except if the class comes from Scala 2
          // because it adds extension methods before pickling.
-          if (!(valueClass.is(Scala2x)))
+          if !valueClass.is(Scala2x, butNot = Scala2Tasty) then


I fixed this issue in this commit to be able to test the attribute in scala2-library-tasty-tests/src/Main.scala

sjrd

One last comment, otherwise LGTM

sjrd · 2023-10-26T13:24:04Z

tasty/src/dotty/tools/tasty/TastyFormat.scala

+
+Standard Section: "Attributes" Attribute*
+```none
+  Attribute     = FLAGattr UTF8             // attributes match the regex [a-zA-Z0-9]+


I might not have been clear enough here about what I meant by specifying what attributes we can have. I suggest we keep an actual list of the possible UTF-8 strings we can find here, and is their meaning. So something like:

Suggested change

Attribute = FLAGattr UTF8 // attributes match the regex [a-zA-Z0-9]+

Attribute = FLAGattr UTF8 // attributes match the regex [a-zA-Z0-9]+

Possible values of FLAGattr attributes are:

* Scala2StandardLibrary This TASTy contains code from the Scala 2 standard library

Then later on we can also add:

* ExplicitNulls This TASTy uses the explicit-nulls variant of the type system

if we do this then why store utf-8 at all, and go fully to tags

Perhaps there's no real difference, indeed.

I will use the tags only as we currently only need them for core Scala features. If we need we can add a tag that can store arbitrary UTF-8 later.

sjrd · 2023-10-26T15:25:52Z

compiler/src/dotty/tools/dotc/core/tasty/AttributePickler.scala

@@ -10,18 +10,15 @@ import java.nio.charset.StandardCharsets
 object AttributePickler:

  def pickleAttributes(
-    attributes: List[String],
+    scala2StandardLibrary: Boolean,


I suggest you create a separate immutable class Attributes in this package that bundles all the possible attributes as fields. pickleAttributes would take an Attributes as argument and AttributeUnpickler would read and probably directly expose an instance of Attributes.

nicolasstucki · 2023-10-27T07:25:24Z

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

+    attributeUnpicklerOpt.exists(_.attributes.scala2StandardLibrary)
+
+  /** This dependency was compiled with explicit nulls enabled */
+  // TODO Use this to tag the symbols of this dependency as compiled with explicit nulls (see use of unpicklingScala2Library).


Should we tag the top-level class with an annotation?

I will need help from someone familiar with explicit nulls to finish this side of the implementation. We can do that in another PR. I could also remove this attribute temporarily until we have the full implementation.

What matters for now is that the information gets stored. If it's not used by dotc at the moment, then we don't care.

nicolasstucki · 2023-10-27T07:36:07Z

tasty/src/dotty/tools/tasty/TastyFormat.scala

+Standard Section: "Attributes" Attribute*
+```none
+  Attribute     = SCALA2STANDARDLIBRARYattr
+                  EXPLICITNULLSattr


I was wondering if this is the correct way to define experimental language attributes. We might end up at some point with attributes that are never used in stable code. On the other hand, we make sure that all attributes that we ever use are unambiguous.

Explicit-nulls is not experimental, at least not in the meaning of @experimental. For historical reasons, it benefits from a favored treatment and is allowed in stable, non-experimental builds (it only needs the flag).

compiler/src/dotty/tools/dotc/core/tasty/AttributePickler.scala

sjrd · 2023-10-27T14:29:20Z

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

+    attributeUnpicklerOpt.exists(_.attributes.scala2StandardLibrary)
+
+  /** This dependency was compiled with explicit nulls enabled */
+  // TODO Use this to tag the symbols of this dependency as compiled with explicit nulls (see use of unpicklingScala2Library).


What matters for now is that the information gets stored. If it's not used by dotc at the moment, then we don't care.

nicolasstucki · 2023-11-06T18:31:33Z

Squashed and rebased

dwijnand added the needs-minor-release This PR cannot be merged until the next minor release label Sep 26, 2023

nicolasstucki force-pushed the add-attributes-section-to-tasty branch 2 times, most recently from 1c146f9 to 9481cf9 Compare September 27, 2023 13:21

nicolasstucki changed the title ~~Add attributes section to TASTy~~ Add attributes section to TASTy and use it for Stdlib TASTy Sep 27, 2023

nicolasstucki self-assigned this Sep 27, 2023

nicolasstucki marked this pull request as ready for review September 27, 2023 15:11

nicolasstucki removed their assignment Sep 27, 2023

nicolasstucki added this to the 3.4.0 milestone Sep 27, 2023

nicolasstucki requested a review from sjrd September 27, 2023 15:12

nicolasstucki assigned sjrd Sep 27, 2023

nicolasstucki added the release-notes Should be mentioned in the release notes label Sep 29, 2023

nicolasstucki force-pushed the add-attributes-section-to-tasty branch from 645d7f5 to 4e12301 Compare October 12, 2023 13:55

sjrd requested changes Oct 25, 2023

View reviewed changes

nicolasstucki force-pushed the add-attributes-section-to-tasty branch 5 times, most recently from 6785fa8 to 2ccdcc1 Compare October 25, 2023 15:51

bishabosha reviewed Oct 25, 2023

View reviewed changes

compiler/src/dotty/tools/dotc/core/tasty/AttributeUnpickler.scala Outdated Show resolved Hide resolved

nicolasstucki force-pushed the add-attributes-section-to-tasty branch from 2ccdcc1 to 51095d0 Compare October 26, 2023 07:17

nicolasstucki requested a review from smarter October 26, 2023 11:38

nicolasstucki assigned smarter Oct 26, 2023

nicolasstucki requested a review from sjrd October 26, 2023 11:38

nicolasstucki commented Oct 26, 2023

View reviewed changes

sjrd requested changes Oct 26, 2023

View reviewed changes

sjrd reviewed Oct 26, 2023

View reviewed changes

nicolasstucki commented Oct 27, 2023

View reviewed changes

nicolasstucki assigned bishabosha Oct 27, 2023

nicolasstucki requested review from bishabosha and sjrd October 27, 2023 08:36

sjrd approved these changes Oct 27, 2023

View reviewed changes

nicolasstucki unassigned sjrd Oct 30, 2023

bishabosha approved these changes Nov 6, 2023

View reviewed changes

Add attributes section to TASTy

16966fd

nicolasstucki force-pushed the add-attributes-section-to-tasty branch from 82d25c2 to 16966fd Compare November 6, 2023 18:31

nicolasstucki enabled auto-merge November 6, 2023 18:31

nicolasstucki merged commit 319e865 into scala:main Nov 6, 2023
16 checks passed

nicolasstucki deleted the add-attributes-section-to-tasty branch November 6, 2023 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attributes section to TASTy and use it for Stdlib TASTy #18599

Add attributes section to TASTy and use it for Stdlib TASTy #18599

nicolasstucki commented Sep 26, 2023 •

edited

Loading

nicolasstucki commented Sep 29, 2023

bishabosha commented Oct 17, 2023

sjrd Oct 25, 2023

nicolasstucki Oct 25, 2023

sjrd Oct 25, 2023

nicolasstucki Oct 25, 2023

sjrd commented Oct 25, 2023

nicolasstucki Oct 26, 2023

sjrd left a comment

sjrd Oct 26, 2023

bishabosha Oct 26, 2023

sjrd Oct 26, 2023

nicolasstucki Oct 26, 2023

sjrd Oct 26, 2023

nicolasstucki Oct 26, 2023

nicolasstucki Oct 27, 2023

nicolasstucki Oct 27, 2023

sjrd Oct 27, 2023

nicolasstucki Oct 27, 2023

sjrd Oct 27, 2023

sjrd Oct 27, 2023

nicolasstucki commented Nov 6, 2023

Add attributes section to TASTy and use it for Stdlib TASTy #18599

Add attributes section to TASTy and use it for Stdlib TASTy #18599

Conversation

nicolasstucki commented Sep 26, 2023 • edited Loading

nicolasstucki commented Sep 29, 2023

bishabosha commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd commented Oct 25, 2023

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasstucki commented Nov 6, 2023

nicolasstucki commented Sep 26, 2023 •

edited

Loading