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

DEBUG-2334 upgrade steep & rbs #3950

Merged
merged 2 commits into from
Sep 25, 2024
Merged

DEBUG-2334 upgrade steep & rbs #3950

merged 2 commits into from
Sep 25, 2024

Conversation

p-datadog
Copy link
Contributor

What does this PR do?

This PR upgrades steep and rbs to current versions.

For rbs, current version is somehow a 3.6 preview - I constrained it to a 3.5 release instead of using the preview. Can upgrade to the preview if this is more desirable.

Motivation:

To type check the code tracking component (#3942) I need type definitions for RubyVM::InstructionSequence which I added in ruby/rbs#2027 to upstream. When I tried to use master of rbs in dd-trace-rb I received several type check errors. This PR repairs the errors to permit usage of current steep & rbs.

Additional Notes:

  • defined? returns nil or a string, not a bool. !! was added in several places to cast the return value to a boolean.
  • "anything" in Ruby is apparently not an Object (I guess there is BasicObject as well?). event.rbs was changed from Object to untyped for "any value", I think "untyped" means anything but correct me if this is not the right type to use.
  • The code in tie.rb runs into steep ignores assignments to variables through local scopes soutaro/steep#1231 where steep does not assign correct type to a variable when its value is overwritten from an inner scope. I don't know how to fix this error on our side (hoping to receive some guidance in the upstream issue) and in the mean time I added tie.rb to excluded files. I checked steep 1.6 that is currently being used in master and in my testing it seems to have the same behavior/issue, thus I don't know why we currently aren't receiving this error in master. My theory is the file somehow isn't actually checked (maybe surrounding types aren't resolved as well as with steep 1.7 and as a result steep doesn't attempt to do anything on the Boot return).
  • The change from self.enabled to enabled in appsec fixes the test suite but I don't know why it was self.enabled before.

How to test the change?

Unsure? Have a question? Request a review!

@p-datadog p-datadog requested review from a team as code owners September 25, 2024 11:32
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (2209a7c) to head (9ab6ac6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3950   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files        1303     1303           
  Lines       78139    78139           
  Branches     3894     3894           
=======================================
+ Hits        76461    76464    +3     
+ Misses       1678     1675    -3     
Flag Coverage Δ
97.85% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM. The appsec signature change doesn't look correct, but it seems OK to me to merge this PR and then drop a note to the appsec folks to decide what to do with it.

def self.enabled: -> bool
def enabled: -> bool
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct 🤔. Including this module in a class does not seem to cause a class to have a #enabled method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a case of soutaro/steep#1232.

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM. The appsec signature change doesn't look correct, but it seems OK to me to merge this PR and then drop a note to the appsec folks to decide what to do with it.

@pr-commenter
Copy link

pr-commenter bot commented Sep 25, 2024

Benchmarks

Benchmark execution time: 2024-09-25 15:14:19

Comparing candidate commit 9ab6ac6 in PR branch upgrade-steep with baseline commit 2209a7c in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics.

@p-datadog
Copy link
Contributor Author

Thinking about the appsec settings situation and soutaro/steep#1232, my impression is that steep would not produce anything useful for that entire file anyway given it has a totally wrong understanding of what self for the vast majority of the file. Therefore I reverted the code change back to what it was (self.enabled), and excluded the file from steep.

@p-datadog p-datadog merged commit a6b9175 into master Sep 25, 2024
206 checks passed
@p-datadog p-datadog deleted the upgrade-steep branch September 25, 2024 16:57
@github-actions github-actions bot added this to the 2.4.0 milestone Sep 25, 2024
p-datadog pushed a commit to p-datadog/dd-trace-rb that referenced this pull request Sep 25, 2024
* master:
  DEBUG-2334 upgrade steep & rbs (DataDog#3950)
  [🤖] Update Latest Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/10976621387
@TonyCTHsu TonyCTHsu added the dev/tooling Involves tools (e.g. Rubocop, CodeCov) label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/tooling Involves tools (e.g. Rubocop, CodeCov)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants