Allows early continue/break to error - #475
Open
LemonInTheDark wants to merge 10 commits into
Open
Conversation
The idea here is to walk the code tree, mark blocks that return/continue/break, and then infect up to parent blocks. We need to ensure that we only "elevate" a control flow case if it ALWAYS happens when we are executed. We use fuzzy to do this, we're fuzzy if we are unsure, unfuzzy if we aren't. There's two types of evaluating (and so two patterns) One type, switches/if blocks, we assume all cases are true, and negate them as we go through all our cases. If all cases have something, we do, if not we don't. If we sometimes don't execute (our case doesn't pass, there's no else), then we're fuzzy and so don't matter to our parent. The other type is evaluating flat blocks. for these, we just run through them, and if we ever terminate early, mark ourselves dirty. Fuzzy cases are not allowed to make their parent true, because they do not reflect all possibilities. What I did here was: - fix fuzzy carrying up into flat blocks, which doesn't make any sense. - remove fuzzy setting from break/continue, this was needed because return was handled special by flat block merging, but it isn't anymore so... - added a bunch of tests to try and catch this stuff I did a survey of some ss13 repos (goon, monkey's eris fork, monkey, tg, bubber) and didn't find any false positives. I did find 2 early continues this catches, and one wild one involving a `for(var/i in const to const)` case which it catches
spookydonut
approved these changes
Aug 3, 2026
Guarenteed loops, blocks we are sure will run, should pass their returns
up the chain so for(var/i in 1 to 2) {return} ... marks correctly. They
however, should not pass up control flow stuff that's scoped TO THAT
LOOP like continues and breaks.
I will note it's possible this will produce very rare false negatives with labeled
break/continues but that shit's evil and I'm not interested in engaging
with it
Owner
|
This is a false positive: /proc/test()
for(var/i in 1 to 2)
if(prob(100))
continue
return
returnSays the second Goon has a detection like this: https://github.com/goonstation/goonstation/blob/1f7b567faba9790b8b8500575e88d5b9bf2be803/code/modules/sound/managed_positional_sound.dm#L99-L111 |
Contributor
Author
|
fuck you're right |
…on for loops better The idea is, instead of thinking of flags as either fuzzy or not, we think of them as, for our current scope, things that MIGHT happen or things that WILL happen. So if a return pops up, that WILL happen. But if we return inside an if block, once we're outside that if block returning is something that MIGHT happen. This allows us to pass stuff up out of guarenteed loops (for/do whiles) better. As an example, if a return will happen inside the loop's block, but a break might happen, then that return should pass outside the for loop as something that might happen, rather then something that will. It also, I hope, makes the code a little easier to think about. Might just be a me benifit though.
…houldn't change behavior at all but it feels more honest
…ould-kill-god' of https://github.com/LemonInTheDark/SpacemanDMM into i-come-from-scientists-and-athiests-and-white-men-who-would-kill-god
Contributor
Author
|
Rewrote it to use maybe/will flags, used maybe flags to handle that situation correctly. My goon repo was 2 years out of date it looks like. |
Contributor
Author
|
idk why the compiler is panicking, will look further into it at a later time. |
Contributor
Author
|
Update: I have no idea |
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.
Explanation of control flow parsing because it took me a bit to get
The idea here is to walk the code tree, mark blocks that return/continue/break, and then infect up to parent blocks.
We need to ensure that we only "elevate" a control flow case if it ALWAYS happens when we are executed. We use fuzzy to do this, we're fuzzy if we are unsure, unfuzzy if we aren't.
There's two types of evaluating (and so two patterns)
One type, switches/if blocks, we assume all cases are true, and negate them as we go through all our cases. If all cases have something, we do, if not we don't. If we sometimes don't execute (our case doesn't pass, there's no else), then we're fuzzy and so don't matter to our parent.
The other type is evaluating flat blocks. for these, we just run through them, and if we ever terminate early, mark ourselves dirty.
Fuzzy cases are not allowed to make their parent true, because they do not reflect all possibilities.
What I changed
What I did here was:
Update: More changes
Reworks control flow processing to use bitflags
The idea is, instead of thinking of flags as either fuzzy or not, we think of them as, for our current scope, things that MIGHT happen or things that WILL happen.
So if a return pops up, that WILL happen. But if we return inside an if block, once we're outside that if block returning is something that MIGHT happen.
This allows us to pass stuff up out of guaranteed loops (for/do whiles) better. As an example, if a return will happen inside the loop's block, but a break might happen, then that return should pass outside the for loop as something that might happen, rather then something that will.
It also, I hope, makes the code a little easier to think about. Might just be a me benefit though.
God I hope there's no false positives
I did a survey of some ss13 repos (goon, monkey's eris fork, monkey, tg, bubber, paradise, nebula) and didn't find any false positives.
I did find 2 early continues this catches (an obvious one on monke and the divine light of ifdefs confusing people on goon)
The humble false negative
We don't handle labels correctly, which could lead to continues/breaks being relevant to their parents. I'm going to ignore this because it's stupid and I don't want to have to think about it.