Skip to content
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

Make dead code indexing use a Model to index constants #564

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Jun 19, 2024

Follow-up on #563.

This time, we switch the indexing of constants to Model:

  • Remove constants indexing from Deadcode::Indexer (41250bb)
  • Make the Deadcode::Plugins take Model::SymbolDef when calling on_define_constant (7894f64)

There is a slight behavior change, before we were able to look at the nesting context when defining T::Enum constants. So with this code:

class Foo < T::Enum
  CONST = new
end

We could tell that CONST wasn't defined inside a enums do ... end block and thus ignore CONST within the sorbet plugin.

To avoid carrying the enums do ... end information and putting pressure on the GC, I dropped this feature, so CONST will be marked as ignored.

In practice this case doesn't happen much, this is a regression that I deemed acceptable.

This PR is easier to review commit by commit.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar force-pushed the at-model-deadcode-constants branch from 7894f64 to 074d408 Compare June 20, 2024 14:27
Base automatically changed from at-model-deadcode-modules to main June 20, 2024 17:20
@Morriar Morriar merged commit b47c919 into main Jun 20, 2024
8 checks passed
@Morriar Morriar deleted the at-model-deadcode-constants branch June 20, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants