Skip to content

Conversation

@AlexWaygood
Copy link
Member

Summary

cargo build --release currently fails to compile on main:

error[E0599]: no method named `hit` found for struct `ReleaseStatistics` in the current scope
   --> crates/red_knot/src/cache.rs:22:29
    |
22  |             self.statistics.hit();
    |                             ^^^ method not found in `ReleaseStatistics`
...
145 | pub struct ReleaseStatistics;
    | ---------------------------- method `hit` not found for this struct

error[E0599]: no method named `miss` found for struct `ReleaseStatistics` in the current scope
   --> crates/red_knot/src/cache.rs:25:29
    |
25  |             self.statistics.miss();
    |                             ^^^^ method not found in `ReleaseStatistics`
...
145 | pub struct ReleaseStatistics;
    | ---------------------------- method `miss` not found for this struct

error[E0599]: no method named `hit` found for struct `ReleaseStatistics` in the current scope
   --> crates/red_knot/src/cache.rs:36:33
    |
36  |                 self.statistics.hit();
    |                                 ^^^ method not found in `ReleaseStatistics`
...
145 | pub struct ReleaseStatistics;
    | ---------------------------- method `hit` not found for this struct

error[E0599]: no method named `miss` found for struct `ReleaseStatistics` in the current scope
   --> crates/red_knot/src/cache.rs:41:33
    |
41  |                 self.statistics.miss();
    |                                 ^^^^ method not found in `ReleaseStatistics`
...
145 | pub struct ReleaseStatistics;
    | ---------------------------- method `miss` not found for this struct

This is because in a release build, CacheStatistics is a type alias for ReleaseStatistics, and ReleaseStatistics doesn't have hit() or miss() methods. (In a debug build, CacheStatistics is a type alias for DebugStatistics, which does have those methods.)

Possibly we could make this less likely to happen in the future by making both structs implement a common trait instead of using type aliases that vary depending on whether it's a debug build or not? For now, though, this PR just brings the two structs in sync w.r.t. the methods they expose.

Test Plan

cargo build --release now once again compiles for me locally

@AlexWaygood AlexWaygood added the internal An internal refactor or improvement label Apr 27, 2024
@github-actions
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh merged commit 4769202 into astral-sh:main Apr 27, 2024
@charliermarsh
Copy link
Member

Seems fine for now, merging to get builds passing. Thanks!

@AlexWaygood AlexWaygood deleted the release-me branch April 27, 2024 15:47
charliermarsh added a commit that referenced this pull request Apr 28, 2024
## Summary

We merged a failure here (#11177), and it only takes ~five minutes
anyway (which is shorter than some of our other jobs).
AlexWaygood added a commit that referenced this pull request Apr 30, 2024
…ache` (#11179)

## Summary

This PR changes the `DebugStatistics` and `ReleaseStatistics` structs so
that they implement a common `StatisticsRecorder` trait, and makes the
`KeyValueCache` struct generic over a type parameter bound to that
trait. The advantage of this approach is that it's much harder for the
`DebugStatistics` and `ReleaseStatistics` structs to accidentally grow
out of sync in the methods that they implement, which was the cause of
the release-build failure recently fixed in #11177.

## Test Plan

`cargo test -p red_knot` and `cargo build --release` both continue to
pass for me locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants