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

Handle class attributes used inside concerns #515

Merged
merged 3 commits into from
Sep 28, 2021
Merged

Conversation

vinistock
Copy link
Member

Motivation

Sorbet is not able to identify class attributes created inside included blocks in concerns, which leads to false positives for method missing.

E.g.:

module Concern
  extend ActiveSupport::Concern

  included do
    class_attribute :title
  end
end

class Article
  include Concern
end

Article.title # => Method missing! (false positive)

Tapioca can find these attributes when we find dynamic inclusion and extension and output the complete RBIs.

Implementation

  • Moved the original logic into DynamicMixinCompiler and modified it to keep track of class attributes
  • Started generating class attribute related RBIs inside GeneratedClassMethods and GeneratedInstanceMethods
  • Re-generated Tapioca's own RBIs

Next steps

This PR will handle gems that create class attribute inside concerns, but applications can also do the same. We can leverage the same logic in a DSL generator to handle that case as well and produce the correct RBIs for concerns.

Tests

See included tests.

@vinistock vinistock requested a review from a team September 23, 2021 21:49
lib/tapioca/compilers/symbol_table/symbol_generator.rb Outdated Show resolved Hide resolved
lib/tapioca.rb Outdated Show resolved Hide resolved
lib/tapioca/compilers/symbol_table/symbol_generator.rb Outdated Show resolved Hide resolved
@vinistock
Copy link
Member Author

I have bumped the Sorbet requirement, applied the requested changes and made three fixes:

  • started invoking super is defined inside re-definition of class_attribute
  • added missing instance attribute checks for the empty? method in DynamicMixinCompiler
  • started rejecting names that begin with <# in Reflection#name_of, in order to properly ignore anonymous classes

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I have a couple of minor comments

lib/tapioca/compilers/dynamic_mixin_compiler.rb Outdated Show resolved Hide resolved
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thank you!

@paracycle
Copy link
Member

paracycle commented Sep 28, 2021

Fixes #250, more generally

@paracycle paracycle linked an issue Sep 28, 2021 that may be closed by this pull request
@vinistock
Copy link
Member Author

@paracycle I think that in order to fix #250 we could use the same strategy, but we'd need to also patch attr_reader, attr_accessor and attr_writer to keep track of the generated methods. In the current state, we'll only catch class_attribute.

@vinistock vinistock merged commit 8b4412f into main Sep 28, 2021
@vinistock vinistock deleted the handle_class_attributes branch September 28, 2021 17:50
@paracycle
Copy link
Member

... but we'd need to also patch attr_reader, attr_accessor and attr_writer to keep track of the generated methods.

I know but that is one usage that I would not want us to support since it is a bad usage of the language. The user should feel the friction in that case and fix their code.

On the other hand, for things like class_attribute there is no other way to declare them properly, so we should support that use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attribute accessors defined in concerns are not generated
3 participants