-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Fix internal formula json v3 frozen hash parsing bug #17201
Fix internal formula json v3 frozen hash parsing bug #17201
Conversation
This caused formulae with uses from macos bounds to not load correctly because they tried to modify a frozen hash. It wasn't obvious from the tests because I didn't replicate the real world JSON parsing conditions closely enough. I also had to modify `Cachable#clear_cache` so that it can clear frozen hashes. Error: ``` Error: can't modify frozen Hash: {"since"=>"catalina"} Warning: Removed Sorbet lines from backtrace! Rerun with `--verbose` to see the original backtrace /usr/local/Homebrew/Library/Homebrew/extend/hash/keys.rb:123:in `delete' /usr/local/Homebrew/Library/Homebrew/extend/hash/keys.rb:123:in `block in _deep_transform_keys_in_object!' /usr/local/Homebrew/Library/Homebrew/extend/hash/keys.rb:122:in `each' /usr/local/Homebrew/Library/Homebrew/extend/hash/keys.rb:122:in `_deep_transform_keys_in_object!' /usr/local/Homebrew/Library/Homebrew/extend/hash/keys.rb:48:in `deep_transform_keys!' /usr/local/Homebrew/Library/Homebrew/formulary.rb:230:in `block (2 levels) in load_formula_from_api' /usr/local/Homebrew/Library/Homebrew/formulary.rb:218:in `each' /usr/local/Homebrew/Library/Homebrew/formulary.rb:218:in `block in load_formula_from_api' /usr/local/Homebrew/Library/Homebrew/formulary.rb:304:in `instance_exec' /usr/local/Homebrew/Library/Homebrew/formulary.rb:304:in `block (2 levels) in load_formula_from_api' /usr/local/Homebrew/Library/Homebrew/formula.rb:3664:in `instance_eval' /usr/local/Homebrew/Library/Homebrew/formula.rb:3664:in `stable' /usr/local/Homebrew/Library/Homebrew/formulary.rb:293:in `block in load_formula_from_api' /usr/local/Homebrew/Library/Homebrew/formulary.rb:283:in `initialize' /usr/local/Homebrew/Library/Homebrew/formulary.rb:283:in `new' /usr/local/Homebrew/Library/Homebrew/formulary.rb:283:in `load_formula_from_api' /usr/local/Homebrew/Library/Homebrew/formulary.rb:962:in `load_from_api' /usr/local/Homebrew/Library/Homebrew/formulary.rb:955:in `klass' /usr/local/Homebrew/Library/Homebrew/formulary.rb:569:in `get_formula' /usr/local/Homebrew/Library/Homebrew/formulary.rb:1009:in `factory' /usr/local/Homebrew/Library/Homebrew/dependency.rb:41:in `to_formula' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:46:in `block (2 levels) in formulae_with_no_formula_dependents' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:45:in `each' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:45:in `block in formulae_with_no_formula_dependents' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:39:in `each' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:39:in `formulae_with_no_formula_dependents' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:59:in `unused_formulae_with_no_formula_dependents' /usr/local/Homebrew/Library/Homebrew/utils/autoremove.rb:16:in `removable_formulae' /usr/local/Homebrew/Library/Homebrew/cleanup.rb:693:in `autoremove' /usr/local/Homebrew/Library/Homebrew/cleanup.rb:291:in `clean!' /usr/local/Homebrew/Library/Homebrew/cmd/cleanup.rb:52:in `run' /usr/local/Homebrew/Library/Homebrew/brew.rb:92:in `<main>' ```
215fceb
to
37cbfc4
Compare
# NOTE: We overwrite here instead of using `Hash#clear` to handle frozen hashes. | ||
sig { void } | ||
def clear_cache | ||
cache.clear | ||
overwrite_cache!({}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal JSON v3 format gets parsed into a frozen hash with JSON.parse(string, frozen: true)
and then gets moved into the cache with Cachable#overwrite_cache!
so the cache is frozen in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there are any real downsides to creating a new hash here instead of using Hash#clear
.
Edit: I guess we could also do this if we don't want to create new hashes here.
def clear_cache
cache.frozen? ? overwrite_cache!({}) : cache.clear
end
Thanks @apainintheneck!
I think it's worth modifying tests where you don't need to do this replication but it's loading formulae as close to the usual API path as possible. |
I agree. I think that #16895 will get us most of the way there if/when someone has the time to work on it. Last time I looked into it I realized that it wasn't going to be a quick migration so I gave up on it. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This caused formulae with uses from macos bounds to not load correctly because they tried to modify a frozen hash. It wasn't obvious from the tests because I didn't replicate the real world JSON parsing conditions closely enough. I also had to modify
Cachable#clear_cache
so that it can clear frozen hashes.Follow-up to #17153.
Note: This doesn't affect users at all since nobody uses this other than me right now.
Error: