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

Check for PanicSentinel in SequenceLiteralNode #7385

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jul 25, 2023

Pull Request Description

Fixes #7245.

Vector with a panic

The above picture shows that the vector problem reported by #7245 is fixed.

Important Notes

The fix modifies SequenceLiteralNode to recognize PanicSentinel and throw it as common in other places (like invoke function node, etc.).

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 25, 2023
@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner July 25, 2023 06:14
@JaroslavTulach JaroslavTulach self-assigned this Jul 25, 2023
@JaroslavTulach JaroslavTulach requested a review from kustosz July 25, 2023 06:14
@JaroslavTulach JaroslavTulach changed the title Wip/jtulach/propagate panic sentinel as warning 7245 Propagate panic sentinel as warnings Jul 25, 2023
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jul 25, 2023
@JaroslavTulach JaroslavTulach added CI: Keep up to date Automatically update this PR to the latest develop. and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Jul 25, 2023
@kustosz
Copy link
Contributor

kustosz commented Jul 26, 2023

One thing to note here – a PanicSentinel should not be handleable by any in-language methods or functions. It is purely a hack to get the IDE to not panic when there's a panic. But note that it has different semantics in CLI and IDE and so cannot be a language mechanism (must be an IDE-only mechanism). I think this PR makes it possible to "escape" a sentinel sometimes? Which is bad. It should not be possible to handle a sentinel.

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/PropagatePanicSentinelAsWarning_7245 branch from 3969890 to 0c6900e Compare July 27, 2023 15:49
@JaroslavTulach
Copy link
Member Author

I think this PR makes it possible to "escape" a sentinel sometimes? Which is bad.
It should not be possible to handle a sentinel.

Thank you Marcin. Rewritten to just throw in SequenceLiteralNode.

Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

It's much better than wrapping it in a warning 👍

@4e6
Copy link
Contributor

4e6 commented Jul 28, 2023

Don't forget to update the PR title

@JaroslavTulach JaroslavTulach changed the title Propagate panic sentinel as warnings Check for PanicSentinel in SequenceLiteralNode Jul 28, 2023
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jul 28, 2023
@mergify mergify bot merged commit 14afa9a into develop Jul 28, 2023
@mergify mergify bot deleted the wip/jtulach/PropagatePanicSentinelAsWarning_7245 branch July 28, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PanicSentinel value isn't propagated via Vector & co.
7 participants