[NO-TICKET] Fix `IncompatibleAssignment` steep check #5015

vpellan · 2025-10-30T18:55:06Z

What does this PR do?

Remove IncompatibleAssignment exclusion from the Steepfile and fixes related issues.

Motivation:

Improve typing on dd-trace-rb.

Change log entry

None.

Additional Notes:

How to test the change?

bundle exec rake steep:check

ivoanjo

👍 Thanks for this + left a few notes!

ivoanjo · 2025-10-31T10:06:41Z

lib/datadog/appsec/security_engine/result.rb

          def initialize(duration_ext_ns:, input_truncated:)
            @events = []
-            @actions = @attributes = {}
+            @actions = {}
+            @attributes = {}


I had to do a double-take on this one -- this is not exactly the same as before but in practice these classes are never mutated.

@y9v, @Strech may not be a bad idea to spray a few .freeze calls around here to make it even more obvious this is all supposed to be immutable?

Spot on, indeed a freeze makes sense here

ivoanjo · 2025-10-31T10:10:04Z

lib/datadog/core/utils/time.rb

        #
        # @param unit [Symbol] unit for the resulting value, same as ::Process#clock_gettime, defaults to :float_second
-        # @return [Numeric] timestamp in the requested unit, since some unspecified starting point
+        # @return [Float] timestamp in the requested unit, since some unspecified starting point


Suggested change

# @return [Float] timestamp in the requested unit, since some unspecified starting point

# @return [Numeric] timestamp in the requested unit, since some unspecified starting point

This change is not correct -- if you change the unit you can get non-floats (e.g. try calling it with :second):

[2] pry(main)> Datadog::Core::Utils::Time.get_time => 4634.229061201 [3] pry(main)> Datadog::Core::Utils::Time.get_time(:second) => 4637

Thanks! Not sure why I changed it to Float only, but I think Numeric is too broad. I've changed it to Float|Integer according to https://github.com/ruby/rbs/blob/master/core/process.rbs#L624-L626

Changed in 15a38cb

ivoanjo · 2025-10-31T10:12:45Z

lib/datadog/di/probe_notification_builder.rb

+      # Steep: `Integer ** Integer` can also result in Rational (e.g. 10**-2)
+      # which is why it gives an error if NANOSECONDS is set to 10**9.


Minor: I suggest removing this comment. That's because, if you came across the new code, you'd never ask "Wait, why is this not 10**9 instead of 1_000_000_000, I must know the reason!!" ;)

I do understand the interest for this kind of comment, but this is a comment for the PR! Or at least as a commit message -- because it's a comment about the change from one to another. But going forward, we'll never need to care about this again.

Suggested change

# Steep: `Integer ** Integer` can also result in Rational (e.g. 10**-2)

# which is why it gives an error if NANOSECONDS is set to 10**9.

Applied in 15a38cb

ivoanjo · 2025-10-31T10:15:57Z

lib/datadog/tracing/span_event.rb

+      # Steep: `Integer ** Integer` can also result in Rational (e.g. 10**-2)
+      # which is why it gives an error if MIN_INT64_SIGNED is set to -2**63.


Minor: Same as my note above -- this doesn't particularly need a comment going forward

Suggested change

# Steep: `Integer ** Integer` can also result in Rational (e.g. 10**-2)

# which is why it gives an error if MIN_INT64_SIGNED is set to -2**63.

Applied in 15a38cb

ivoanjo · 2025-10-31T10:21:55Z

lib/datadog/tracing/span_event.rb

+      # DEV: This is incorrect, 2 << 63 - 1 is not the same as 2**63 - 1 (offset by 1).
+      # The correct formula is `(2 << 62) - 1`. Check if it is a breaking change before fixing it.
      MAX_INT64_SIGNED = 2 << 63 - 1


This is a nice catch, and indeed it's not correct -- there is an off-by-one here.

I think it's very much worth fixing, either on this PR or another; let's not just leave a comment here and move on ;)

Fixed in 15a38cb

ivoanjo · 2025-10-31T10:25:31Z

lib/datadog/tracing/span_operation.rb

+        # The only place where start_time_nano is called previously checks if span is stopped,
+        # which checks if @end_time is not nil, and the only place where @end_time is set is in stop,
+        # which also sets @start_time if it wasn't already set.
+        # @type ivar @start_time: ::Time
        @start_time.to_i * 1000000000 + @start_time.nsec


Minor: Hmm... I'm not quite convinced about this override. In particular, it's based on the current state of non-local code, and who's going to remember to update this comment if any of the logic it's commenting on actually changes?

So I think it's better to have a steep:ignore than a "here's some potentially-misleading types" IMO?

I agree, but at the same time, if someone uses that methods in a context where start_time might be nil, this will cause an error. I'll add a guard clause.

Added in 15a38cb

pr-commenter · 2025-10-31T16:17:11Z

Benchmarks

Benchmark execution time: 2025-10-31 18:33:07

Comparing candidate commit 20e89c7 in PR branch vpellan/steep-fix-incompatible-assignment with baseline commit f060ebf in branch master.

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

scenario:line instrumentation - untargeted

🟥 throughput [-5276.863op/s; -5122.993op/s] or [-8.912%; -8.653%]

scenario:method instrumentation

🟥 throughput [-11493.338op/s; -11060.938op/s] or [-6.485%; -6.241%]

scenario:profiling - Allocations (baseline)

🟩 throughput [+274117.005op/s; +287242.169op/s] or [+5.480%; +5.743%]

Co-authored-by: Ivo Anjo <[email protected]>

github-actions · 2025-10-31T18:02:34Z

Typing analysis

Note: Ignored files are excluded from the next sections.

`steep:ignore` comments

This PR introduces 23 steep:ignore comments, and clears 3 steep:ignore comments.

steep:ignore comments (+23-3)

❌ Introduced:

lib/datadog/appsec/context.rb:11
lib/datadog/core/configuration/option_definition.rb:46
lib/datadog/core/configuration/option_definition.rb:124
lib/datadog/core/configuration/options.rb:45
lib/datadog/core/configuration/options.rb:48
lib/datadog/core/configuration/options.rb:52
lib/datadog/core/configuration/options.rb:53
lib/datadog/core/configuration/options.rb:133
lib/datadog/core/rate_limiter.rb:165
lib/datadog/di/el/compiler.rb:26
lib/datadog/di/el/compiler.rb:38
lib/datadog/di/el/compiler.rb:44
lib/datadog/di/el/compiler.rb:47
lib/datadog/di/probe_builder.rb:24
lib/datadog/profiling/collectors/code_provenance.rb:141
lib/datadog/profiling/collectors/info.rb:20
lib/datadog/profiling/http_transport.rb:19
lib/datadog/tracing/contrib/status_range_matcher.rb:12
lib/datadog/tracing/contrib/utils/quantization/hash.rb:15
lib/datadog/tracing/span_operation.rb:293
lib/datadog/tracing/span_operation.rb:320
lib/datadog/tracing/span_operation.rb:392
lib/datadog/tracing/span_operation.rb:556

✅ Cleared:

lib/datadog/core/configuration/options.rb:47
lib/datadog/core/configuration/options.rb:51
lib/datadog/tracing/span_operation.rb:319

Untyped methods

This PR introduces 5 partially typed methods, and clears 4 partially typed methods. It decreases the percentage of typed methods from 51.66% to 51.59% (-0.07%).

Partially typed methods (+5-4)

❌ Introduced:

sig/datadog/core/configuration/option_definition.rbs:88
└── def default_proc: () ?{ (?) [self: Options::GenericSettingsClass] -> untyped } -> void
sig/datadog/core/configuration/option_definition.rbs:92
└── def helper: (Symbol name, *untyped _args) { (?) [self: Options::GenericSettingsClass] -> untyped } -> void
sig/datadog/di/instrumenter.rbs:24
└── def initialize: (untyped settings, Serializer serializer, DI::Logger logger, ?code_tracker: CodeTracker?, ?telemetry: Core::Telemetry::Component?) -> void
sig/datadog/di/serializer.rbs:13
└── def initialize: (untyped settings, untyped redactor, ?telemetry: Core::Telemetry::Component?) -> void
sig/datadog/tracing/contrib/graphql/unified_trace.rbs:94
└── def trace: (Proc callable, String trace_key, (String | nil) resource, ?(^(SpanOperation) -> void)? before, ?(^(SpanOperation) -> void)? after, **untyped kwargs) ?{ (SpanOperation) -> void } -> untyped

✅ Cleared:

sig/datadog/core/configuration/option_definition.rbs:80
└── def helper: (Symbol name, *untyped _args) { (?) [self: Options::GenericSettingsClass] -> untyped } -> void
sig/datadog/di/instrumenter.rbs:24
└── def initialize: (untyped settings, Serializer serializer, DI::Logger logger, ?code_tracker: CodeTracker?, ?telemetry: Core::Telemetry::Component) -> void
sig/datadog/di/serializer.rbs:13
└── def initialize: (untyped settings, untyped redactor, ?telemetry: Core::Telemetry::Component) -> void
sig/datadog/tracing/contrib/graphql/unified_trace.rbs:94
└── def trace: (Proc callable, String trace_key, (String | nil) resource, ?^(SpanOperation) -> void before, ?^(SpanOperation) -> void after, **untyped kwargs) ?{ (SpanOperation) -> void } -> untyped

Untyped other declarations

This PR introduces 5 partially typed other declarations, and clears 1 untyped other declaration and 5 partially typed other declarations. It increases the percentage of typed other declarations from 65.57% to 65.65% (+0.08%).

Untyped other declarations (+0-1)

✅ Cleared:

sig/datadog/version.rbs:13
└── STRING: untyped

Partially typed other declarations (+5-5)

❌ Introduced:

sig/datadog/core/configuration/option_definition.rbs:55
└── attr_reader type_options: Hash[Symbol, untyped]
sig/datadog/core/configuration/option_definition.rbs:77
└── @type_options: Hash[Symbol, untyped]
sig/datadog/di/probe.rbs:19
└── @template_segments: Array[untyped]?
sig/datadog/di/probe.rbs:50
└── attr_reader template_segments: Array[untyped]?
sig/datadog/tracing/metadata/tagging.rbs:6
└── ENSURE_AGENT_TAGS: ::Hash[untyped, bool]

✅ Cleared:

sig/datadog/core/configuration/option_definition.rbs:43
└── attr_reader type_options: Hash[Symbol, untyped]
sig/datadog/core/configuration/option_definition.rbs:65
└── @type_options: Hash[Symbol, untyped]
sig/datadog/di/probe.rbs:19
└── @template_segments: Array[untyped]
sig/datadog/di/probe.rbs:50
└── attr_reader template_segments: Array[untyped]
sig/datadog/tracing/metadata/tagging.rbs:6
└── ENSURE_AGENT_TAGS: ::Hash[untyped, true]

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept to the end of the line to remove it from the stats.

datadog-official · 2025-10-31T18:18:10Z

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage
• Patch Coverage: 111.11%
• Total Coverage: 98.55% (+0.01%)

View detailed report

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 20e89c7 | Docs | Datadog PR Page | Was this helpful? Give us feedback!}

p-datadog

I don't understand why 2**63-1 does not fit into a signed int64, can you clarify?

p-datadog · 2025-11-03T10:55:22Z

lib/datadog/appsec/context.rb

    class Context
-      ActiveContextError = Class.new(StandardError)
+      # Steep: https://github.com/soutaro/steep/issues/1880
+      ActiveContextError = Class.new(StandardError) # steep:ignore IncompatibleAssignment


If you use class ActiveContextError < StandardError does steep then work?

Yes it does but I remember that @Strech preferred to keep it as is, which is why I've added a comment with the issue, and disabled it

I think type definition as such should work, no? But code could stay the same

p-datadog

I got it now, << vs **

Strech · 2025-11-03T12:31:23Z

lib/datadog/appsec/security_engine/result.rb

          def initialize(duration_ext_ns:, input_truncated:)
            @events = []
-            @actions = @attributes = {}
+            @actions = {}
+            @attributes = {}


Spot on, indeed a freeze makes sense here

Strech · 2025-11-03T12:32:35Z

lib/datadog/core/configuration/option_definition.rb

        class Builder
-          InvalidOptionError = Class.new(StandardError)
+          # Steep: https://github.com/soutaro/steep/issues/1880
+          InvalidOptionError = Class.new(StandardError) # steep:ignore IncompatibleAssignment


Sorry, but will this work?

class InvalidOptionError < StandardError end

Strech · 2025-11-03T12:33:05Z

lib/datadog/core/utils/time.rb

        #
        # @param unit [Symbol] unit for the resulting value, same as ::Process#clock_gettime, defaults to :float_second
-        # @return [Numeric] timestamp in the requested unit, since some unspecified starting point
+        # @return [Float|Integer] timestamp in the requested unit, since some unspecified starting point


vpellan requested review from a team as code owners October 30, 2025 18:55

vpellan requested review from mabdinur and removed request for a team October 30, 2025 18:55

github-actions bot added core Involves Datadog core libraries integrations Involves tracing integrations profiling Involves Datadog profiling appsec Application Security monitoring product tracing labels Oct 30, 2025

ivoanjo approved these changes Oct 31, 2025

View reviewed changes

Base automatically changed from vpellan/steep-fix-different-method-parameter-kind to master October 31, 2025 17:58

vpellan and others added 3 commits October 31, 2025 14:01

Fix OptionDefinition typing (accounting for steep issues)

28d944b

Fix IncompatibleAssignment steep rule

2175887

Apply suggestions

20e89c7

Co-authored-by: Ivo Anjo <[email protected]>

vpellan force-pushed the vpellan/steep-fix-incompatible-assignment branch from 15a38cb to 20e89c7 Compare October 31, 2025 18:02

p-datadog requested changes Nov 3, 2025

View reviewed changes

p-datadog approved these changes Nov 3, 2025

View reviewed changes

Strech approved these changes Nov 3, 2025

View reviewed changes

	# @return [Float] timestamp in the requested unit, since some unspecified starting point
	# @return [Numeric] timestamp in the requested unit, since some unspecified starting point

		# Steep: `Integer Integer` can also result in Rational (e.g. 10-2)
		# which is why it gives an error if NANOSECONDS is set to 10**9.

		# Steep: `Integer Integer` can also result in Rational (e.g. 10-2)
		# which is why it gives an error if MIN_INT64_SIGNED is set to -2**63.

[NO-TICKET] Fix IncompatibleAssignment steep check #5015

Are you sure you want to change the base?

[NO-TICKET] Fix IncompatibleAssignment steep check #5015

Conversation

vpellan commented Oct 30, 2025

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpellan Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pr-commenter bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:line instrumentation - untargeted

scenario:method instrumentation

scenario:profiling - Allocations (baseline)

Uh oh!

github-actions bot commented Oct 31, 2025

Typing analysis

steep:ignore comments

Untyped methods

Untyped other declarations

Uh oh!

datadog-official bot commented Oct 31, 2025

Uh oh!

p-datadog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Strech Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-datadog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

[NO-TICKET] Fix `IncompatibleAssignment` steep check #5015

[NO-TICKET] Fix `IncompatibleAssignment` steep check #5015

vpellan Oct 31, 2025 •

edited

Loading

pr-commenter bot commented Oct 31, 2025 •

edited

Loading

`steep:ignore` comments

Strech Nov 3, 2025 •

edited

Loading