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

Allow non-hot values in SeqLiterals #14794

Merged
merged 2 commits into from
Apr 6, 2022
Merged

Conversation

Xavientois
Copy link
Contributor

Closes #14460, #14751

Do not ensure that elements in a SeqLiteral are Hot.

Review by @liufengyun

@Xavientois Xavientois requested a review from liufengyun March 27, 2022 20:47
@Xavientois
Copy link
Contributor Author

@liufengyun @olhotak I have not yet implemented the parametricity solution we discussed, as this change appears to solve all of the enum cases we were looking at (Array.apply and AnyRef).

This currently passes all of the enum test cases, but fails three neg tests:

  • tests/init/neg/scodec.scala
  • tests/init/neg/early-promote.scala
  • tests/init/neg/leak-warm.scala

Since we want List.apply to allow cold arguments, it looks like leak-warm should no longer cause an error.

I am less certain about the desired behaviour for the other two tests. Should they still trigger an error?

@liufengyun
Copy link
Contributor

Thanks Josh!

I am less certain about the desired behaviour for the other two tests. Should they still trigger an error?

For SeqLiterals, we need to return a join of the argument values instead of Hot. The current change simply skips the check of argument values, thus it is problematic.

@Xavientois
Copy link
Contributor Author

For SeqLiterals, we need to return a join of the argument values instead of Hot. The current change simply skips the check of argument values, thus it is problematic.

I think I may have originally misunderstood. What do you mean by the join of the values? Should I try to promote them?

@liufengyun
Copy link
Contributor

For SeqLiterals, we need to return a join of the argument values instead of Hot. The current change simply skips the check of argument values, thus it is problematic.

I think I may have originally misunderstood. What do you mean by the join of the values? Should I try to promote them?

I mean you can return Result(ress.map(_.value).join, ress.flatMap(_.errors)). The logic in call will take care of the promotion check.

@Xavientois Xavientois force-pushed the parametricity-for-safe-init-enums branch 2 times, most recently from 7a79695 to c01c30a Compare April 4, 2022 18:27
@Xavientois Xavientois marked this pull request as ready for review April 4, 2022 18:27
@Xavientois Xavientois force-pushed the parametricity-for-safe-init-enums branch 3 times, most recently from 9858f5f to 5aa4a79 Compare April 4, 2022 21:05
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

@Xavientois Xavientois force-pushed the parametricity-for-safe-init-enums branch from 5aa4a79 to ff64ce8 Compare April 5, 2022 01:11
Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

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

LGTM. There's a merge conflict in a .check file that needs to be resolved.

@olhotak olhotak enabled auto-merge April 5, 2022 09:38
Closes #14460, #14751

Do not ensure that elements in a SeqLiteral are Hot.

Review by @liufengyun
@Xavientois Xavientois force-pushed the parametricity-for-safe-init-enums branch from ff64ce8 to da09d3a Compare April 5, 2022 14:47
@Xavientois Xavientois force-pushed the parametricity-for-safe-init-enums branch from 91e6f52 to 6ff1edd Compare April 5, 2022 17:52
@olhotak olhotak merged commit 73cda0c into main Apr 6, 2022
@olhotak olhotak deleted the parametricity-for-safe-init-enums branch April 6, 2022 00:29
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inner enum declarations cause initialization-checker error
4 participants