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

Fix #11861 - hash nested calls to inline definitions #12931

Merged
merged 8 commits into from
Dec 22, 2021

Conversation

bishabosha
Copy link
Member

fixes #11861

@informarte
Copy link

Will this fix make it into Scala 3.0.2?

@FabioPinheiro
Copy link
Contributor

@informarte You mean 3.0.3 (3.0.2 was already released)

@bishabosha
Copy link
Member Author

@informarte You mean 3.0.3 (3.0.2 was already released)

next release will be 3.1.0

Will this fix make it into Scala 3.0.2?

Yes, for 3.1.0, I can go back and address the comments - we will see if the implementation is correct

@bishabosha bishabosha added backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Sep 9, 2021
@informarte
Copy link

@bishabosha, @smarter, what are your plans for this PR? I see tests, I see a green build, I see comments that were addressed, so what keeps you from merging this fix?

@bishabosha
Copy link
Member Author

what are your plans for this PR?

I just need to add some assertion tests and should be good to go

@SethTisue
Copy link
Member

I'm interested in seeing this land, since it would help me in my own compiler hacking. (Happy to help, even, if there's something I can do.)

@bishabosha
Copy link
Member Author

bishabosha commented Nov 18, 2021

Hey @SethTisue, I will dedicate myself to this tomorrow - mostly the confusion comes from this comment - #12931 (comment) where I got an error from building - seemingly that incremental compilation might still be incorrect - the remaining work then is to add some assertions and maybe experiment to trigger the same error if it was not a fluke

@bishabosha
Copy link
Member Author

@smarter I have changed to implementation to only use apiDefinition, and have adapted apiAnnotations to control the loop condition for inline body hashing

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

A few comments still but I think we're getting there!

compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala Outdated Show resolved Hide resolved

def mixInlineParam(p: Symbol): Unit =
if inlineOrigin.exists && p.is(Inline) then
inlineExtras += hashInlineParam(p)
Copy link
Member

Choose a reason for hiding this comment

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

Potential generalization that could be done later: instead of having a hash just for inline params, have a hash for all the potentially pickled flags: if we define a val PickledFlags union of everything that appears in TreePickler#pickleFlags we could do h = mix(h, p.flags & PickledFlags) (though like in tasty pickling we need to be careful about Stable cf 456e7c6)

compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala Outdated Show resolved Hide resolved
@smarter smarter assigned bishabosha and unassigned smarter Dec 20, 2021
@bishabosha bishabosha assigned smarter and unassigned bishabosha Dec 21, 2021
@bishabosha bishabosha force-pushed the fix-11861 branch 2 times, most recently from 94294a9 to 18187bf Compare December 21, 2021 16:07
Copy link
Member

@smarter smarter 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/sbt/ExtractAPI.scala Outdated Show resolved Hide resolved
@smarter smarter assigned bishabosha and unassigned smarter Dec 22, 2021
@bishabosha bishabosha assigned smarter and unassigned bishabosha Dec 22, 2021
@bishabosha bishabosha assigned bishabosha and unassigned smarter Dec 22, 2021
@bishabosha bishabosha merged commit c94b333 into scala:master Dec 22, 2021
@bishabosha bishabosha deleted the fix-11861 branch December 22, 2021 11:47
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Dec 22, 2021
@smarter
Copy link
Member

smarter commented Dec 22, 2021

@bishabosha bishabosha added the release-notes label 2 hours ago

I think the convention we used is that if something should be mentioned in the release note, the PR description should be updated with the text to be inserted in the release note, could you handle that?

@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zinc invalidation when nested inline method changes
6 participants