Repository navigation
B3: EmptyBlock keeps finally statements and switch selector side effects, and leaves synchronized alone - #1117
Merged
Merged
Conversation
…ized` and `switch` Replace an empty `try` with its `finally` statements, extract side effects from the `synchronized` lock and the `switch` selector as is done for `if` conditions, leave conditions alone whose side effects would become unconditional (right of `&&`/`||`, ternaries), and do not invert floating-point comparisons, which changes the result for NaN. Fixes B3 of #1116
15 of 41 tasks
Acquiring the monitor waits for other holders and establishes a happens-before edge, so removing even an empty block changes behaviour.
timtebeek
changed the title
EmptyBlock keeps finally statements and side effects of synchronized and switchEmptyBlock keeps finally statements and switch selector side effects, and leaves synchronized alone
…visit Replacements now come from the already-visited children, so nested fixes inside `finally` survive in one cycle without an extra after-visit per statement. Flattened `finally` statements are only re-indented rather than auto-formatted, and `finally` stays a block when it declares pattern variables or ends in comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes B3 of #1116:
EmptyBlock(inCommonStaticAnalysisandCodeCleanup) dropped code that runs, not just empty blocks.try { } finally { r.run(); }was deleted whole. It is now replaced by thefinallystatements, or by a plain{ … }block when those declare variables, classes or pattern variables that could clash with later declarations, or when the block ends in comments that would otherwise be lost.switch (sel()) {}was deleted along with its selector call. It now keeps the side effects, the same way the empty-ifpath already did.synchronizedblocks are no longer removed at all. Acquiring the monitor waits for other holders and establishes a happens-before edge, so an empty block can be a deliberate barrier, and no rewrite preserves that.ifpath hoisted every side effect out of the condition, soif (m.find() && m.group(1) != null) {}becamem.find(); m.group(1);, which throws when nothing matches. A condition is now left alone whengetSideEffects()would make a call unconditional (right of&&/||) or would miss one (ternaries, array indexes, …). Side effects on the left of&&are still extracted.if (d > 0) {} else { … }tod <= 0changes the result for NaN, so relational comparisons with afloat/double(or untyped) operand are no longer inverted.==/!=inversion is unaffected.Verified with
./gradlew test --tests EmptyBlockTest(new probe tests for each case fail onmainand pass here) and a full./gradlew test. The only failures were thepython()tests, which can't install the pip dev build on a local Python 3.9, and a few Kotlin tests of recipes that don't useEmptyBlockVisitor, which flaked and passed on rerun.