Fix Rails engine symbol loading #1028
Merged
Conversation
This file contains 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
Since [this commit](ed9e590) we have not been loading Rails engine symbols from gems. The commit started filtering Rails engines by looking up the name of the engine in the payload symbol table which was not the correct thing to do and would never return an engine for consideration. What should be happening is to find the engine that is exported from a gem, and to collect all the files that belong to that engine, so that Sorbet can find the symbols. Generally, this would not be causing a problem if all the constants loaded from an engine were under the gem's top level namespace, since we walk the namespace down and would discover those constants (since there was, hopefully, no problem with loading the engine files). If the engine was exporting a top-level constant, though, we would not be able to catch that since we never scanned the symbols. This was happening, for example, for the `view_component` gem's [`PreviewHelper` module](https://github.com/github/view_component/blob/8dd9063d0b74125c0e20222ee76e64faac6d13a2/app/helpers/preview_helper.rb).
rafaelfranca
approved these changes
Jul 1, 2022
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.
Motivation
Fixes #1027
Since this commit we have not been loading Rails engine symbols from gems. The commit started filtering Rails engines by looking up the name of the engine in the payload symbol table which was not the correct thing to do and would never return an engine for consideration.
What should be happening is to find the engine that is exported from a gem, and to collect all the files that belong to that engine, so that Sorbet can find the symbols.
Generally, this would not be causing a problem if all the constants loaded from an engine were under the gem's top level namespace, since we walk the namespace down and would discover those constants (since there was, hopefully, no problem with loading the engine files). If the engine was exporting a top-level constant, though, we would not be able to catch that since we never scanned the symbols. This was happening, for example, for the
view_componentgem'sPreviewHelpermodule.Implementation
Instead of memoizing
engine_symbols, we are now memoizing the list ofengines, andengine_symbolsmethod takes the gem object to filter the list of engines to find the one exported by the gem.Tests
Added a test that creates a Rails engine and defines both top-level and namespaced classes within the engine.