[compiler] Validate against mutable functions being frozen #33079

josephsavona · 2025-05-01T08:43:59Z

Stack from ghstack (oldest at bottom):

Adds validation for the case from the previous PR. This is actually just revisiting a validation I built a while ago to disallow mutable functions at frozen locations, but I tried to make it much more precise this time to avoid false positives.

We detect function expressions which are known mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of refs or mutations that we aren't sure about (untyped function calls for example). Any such known-mutating function passed to a location with a Freeze effect is reported as a validation error.

This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default.

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. [ghstack-poisoned]

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: 40f98b4 Pull Request resolved: #33079

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. [ghstack-poisoned]

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: 40f98b4 Pull Request resolved: #33079

josephsavona · 2025-05-02T01:12:34Z

For Meta folks: this validation catches some invalid code that came up today: https://fburl.com/workplace/cbnjb4xg

Adds validation for the case from the previous PR. This is actually just revisiting a validation I built a while ago to disallow mutable functions at frozen locations, but I tried to make it much more precise this time to avoid false positives. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. [ghstack-poisoned]

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: db0a94e Pull Request resolved: #33079

Adds validation for the case from the previous PR. This is actually just revisiting a validation I built a while ago to disallow mutable functions at frozen locations, but I tried to make it much more precise this time to avoid false positives. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. [ghstack-poisoned]

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: 075a731 Pull Request resolved: #33079

mvitousek

Nice! I like this as analogous with the existing validation against writing to locals in escaping functions, would definitely be inclined to turn this on.

josephsavona · 2025-05-02T23:26:19Z

@mvitousek Yeah that’s what I was thinking too. Unless there is an obvious issue when I test internally I’ll turn this on by default right away.

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: 075a731 Pull Request resolved: #33079

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: 075a731 Pull Request resolved: #33079 DiffTrain build for [0db8db1](0db8db1)

…ctions Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned]

…ctions Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. ghstack-source-id: 39cdff2 Pull Request resolved: #33113

This revisits a validation I built a while ago, trying to make it more strict this time to ensure that it's high-signal. We detect function expressions which are *known* mutable — they definitely can modify a variable defined outside of the function expression itself (modulo control flow). This uses types to look for known Store and Mutate effects only, and disregards mutations of effects. Any such function passed to a location with a Freeze effect is reported as a validation error. This is behind a flag and disabled by default. If folks agree this makes sense to revisit, i'll test out internally and we can consider enabling by default. ghstack-source-id: 075a731 Pull Request resolved: facebook#33079 DiffTrain build for [0db8db1](facebook@0db8db1)

…nMutableFunctions" Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned]

…teNoFreezingKnownMutableFunctions" Found when testing the new validation from #33079 internally. I haven't fully debugged, but somehow the combination of the effect function *accessing* a ref and also calling a second function which has a purely local mutation triggers the validation. Even though the called second function only mutates local variables. If i remove the ref access in the effect function, the error goes away. Anyway I'll keep debugging, putting up a repro for now. [ghstack-poisoned]