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

Store source file in TASTY attributes #18948

Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Nov 16, 2023

Add source file to TASTy attributes. This is a first step towards removing the @SourceFile annotation.

Release notes

Tools that read TASTy need to know that the source file must be unpickled from TASTy attributes.

@nicolasstucki nicolasstucki self-assigned this Nov 16, 2023
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch 2 times, most recently from 979699d to a7e774a Compare November 17, 2023 12:48
@nicolasstucki nicolasstucki added release-notes Should be mentioned in the release notes needs-minor-release This PR cannot be merged until the next minor release labels Nov 17, 2023
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch 6 times, most recently from 34629c2 to 028219c Compare November 23, 2023 15:16
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch 5 times, most recently from 6a8eaff to 5268c6f Compare November 24, 2023 12:21
nicolasstucki added a commit that referenced this pull request Nov 27, 2023
We generalize the internal encoding of `Attributes` to be a list of
tags. Then we add add helper methods to have simpler ways to interact
with this abstraction using booleans. This implies that the
pickling/unpickling can be agnostic of the semantics of each tag.
Therefore reducing the number of places that need to be updated when we
add a new tag.

Useful for #19074, #19033, and #18948.
nicolasstucki added a commit to dotty-staging/dotty that referenced this pull request Nov 27, 2023
We generalize the internal encoding of `Attributes` to be a list of
tags. Then we add add helper methods to have simpler ways to interact
with this abstraction using booleans. This implies that the
pickling/unpickling can be agnostic of the semantics of each tag.
Therefore reducing the number of places that need to be updated when we
add a new tag.

Useful for scala#19074, scala#19033, and scala#18948.
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch 3 times, most recently from 27881d3 to 8f71739 Compare November 27, 2023 13:23
bishabosha added a commit that referenced this pull request Nov 27, 2023
We add a `Utf8` encoding to the grammar. This should not to be confused
with the `UTF8` name tag. This mistake was made in the `Comment` format.
We also add corresponding `writeUtf8` and `readUtf8` methods to the
`TastyBuffer`.

This is also useful for #18948
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch from 9e01840 to 50a8ca6 Compare November 27, 2023 17:21
```

Standard Section: "Attributes" Attribute*
```none
Attribute = SCALA2STANDARDLIBRARYattr
EXPLICITNULLSattr
SOURCEFILEattr Utf8
Copy link
Member

Choose a reason for hiding this comment

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

should this be raw utf8 or a nametable reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch from 30ad31e to adf7e3f Compare November 28, 2023 08:00
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch 8 times, most recently from cad4d30 to b52d156 Compare November 30, 2023 13:59
@nicolasstucki nicolasstucki marked this pull request as ready for review December 1, 2023 06:41
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch from b52d156 to 791c6fd Compare December 1, 2023 06:43
else if isStringAttrTag(tag) then
val utf8Ref = readNameRef()
val value = nameAtRef(utf8Ref).toString
stringTagValue += tag -> value
Copy link
Member

@bishabosha bishabosha Dec 1, 2023

Choose a reason for hiding this comment

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

perhaps we should still validate the tags - there's a (hopefully unlikely) possibility of someone stuffing the attribute section with duplicates/invalid tags in the correct range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invalid tags are checked with isStringAttrTag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should add to the spec the fact that attribute tags cannot be repeated. I could also specify that they must be in order to make it reproducible.

This introduces the first non-boolean attribute to the TASTY format. We
Split the tags into a set of boolean tags and string tags to handle This
new kind of attribute. We keep some tags as unknown to allow for additional
kinds of tags in the future.
Also refactor accesses to the tasty attributes to get them directly from
the compilation unit info instead of the unpickler.
The only missing step is in the TODO added in this commit.
@nicolasstucki nicolasstucki force-pushed the add-tasty-source-file-attribute branch from 0631d67 to e942cd0 Compare December 1, 2023 16:58
@bishabosha bishabosha merged commit d96e9e4 into scala:main Dec 1, 2023
19 checks passed
@bishabosha bishabosha deleted the add-tasty-source-file-attribute branch December 1, 2023 21:40
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
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 release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants