Skip to content

Prevent crash during ActiveRecord const check #328

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2024
Merged

Prevent crash during ActiveRecord const check #328

merged 1 commit into from
Apr 16, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Apr 13, 2024

I'm not exactly sure how it was triggered, but I had crash for this:

undefined method `<' for an instance of T::Private::Types::TypeAlias (NoMethodError)

I'll add a test if I'm able to reproduce.

@andyw8 andyw8 requested a review from a team as a code owner April 13, 2024 21:39
@andyw8 andyw8 requested review from egiurleo and KaanOzkan April 13, 2024 21:39
andyw8
andyw8 commented Apr 13, 2024
View reviewed changes
lib/ruby_lsp/ruby_lsp_rails/server.rb
@@ -104,6 +104,17 @@ def resolve_database_info_from_model(model_name)
rescue => e
{ error: e.full_message(highlight: false) }
end

sig { params(const: T.untyped).returns(T::Boolean) }
def active_record_model?(const)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted out since this was getting a bit lengthy.

@andyw8 andyw8 force-pushed the andyw8/ensure-const-responds-to-kind-check branch from 6f0cbb7 to 7a6887b Compare April 13, 2024 21:41
@andyw8 andyw8 added the bug Something isn't working label Apr 13, 2024
vinistock
vinistock reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

This happens because certain classes override the < operator. We already handle this in Tapioca, by inverting the check

# Instead of doing
something < ActiveRecord::Base
# Use
ActiveRecord::Base > something

That should probably be enough to fix it.

@andyw8 andyw8 force-pushed the andyw8/ensure-const-responds-to-kind-check branch from 7a6887b to 435edab Compare April 15, 2024 19:33
@andyw8 andyw8 requested a review from a team as a code owner April 15, 2024 19:33
@andyw8 andyw8 requested review from vinistock and aryan-soni April 15, 2024 19:33
@andyw8 andyw8 force-pushed the andyw8/ensure-const-responds-to-kind-check branch from 435edab to dcbb252 Compare April 15, 2024 19:34
@andyw8 andyw8 force-pushed the andyw8/ensure-const-responds-to-kind-check branch from dcbb252 to 55499c3 Compare April 15, 2024 19:35
@andyw8
Copy link
Contributor Author

andyw8 commented Apr 15, 2024

Updated based on @vinistock's suggestion, and I added a test.

st0012
st0012 approved these changes Apr 15, 2024
View reviewed changes
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Solution looks great 👍 It'd be great to have some test cases around it if the setup is not too complicated.

@st0012
Copy link
Member

st0012 commented Apr 15, 2024

BTW, is bug the right label? I thought we use bugfix?

@andyw8 andyw8 added bugfix This PR fixes an existing bug and removed bug Something isn't working labels Apr 15, 2024
@andyw8
Copy link
Contributor Author

andyw8 commented Apr 15, 2024

I corrected the label.

It'd be great to have some test cases around it if the setup is not too complicated.

I added a test with my update, is there something else you had in mind?

@st0012
Copy link
Member

st0012 commented Apr 15, 2024

is there something else you had in mind?

Oh sorry I must've clicked into just a specific commit. All good then 👍

@andyw8 andyw8 merged commit fee564a into main Apr 16, 2024
@andyw8 andyw8 deleted the andyw8/ensure-const-responds-to-kind-check branch April 16, 2024 13:15
@andyw8 andyw8 mentioned this pull request Feb 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

@egiurleo egiurleo egiurleo approved these changes

@vinistock vinistock vinistock approved these changes

@KaanOzkan KaanOzkan Awaiting requested review from KaanOzkan KaanOzkan was automatically assigned from Shopify/ruby-dev-exp

@aryan-soni aryan-soni Awaiting requested review from aryan-soni aryan-soni is a code owner automatically assigned from Shopify/ruby-dx

Assignees
No one assigned
Labels
bugfix This PR fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andyw8 @st0012 @egiurleo @vinistock