Skip to content

Comments

*: enable borrow_interior_mutable_const clippy lint#31875

Merged
teskje merged 2 commits intoMaterializeInc:mainfrom
teskje:clippy-borrow_interior_mutable_const
Mar 13, 2025
Merged

*: enable borrow_interior_mutable_const clippy lint#31875
teskje merged 2 commits intoMaterializeInc:mainfrom
teskje:clippy-borrow_interior_mutable_const

Conversation

@teskje
Copy link
Contributor

@teskje teskje commented Mar 12, 2025

This PR enables a new clip lint, borrow_interior_mutable_const. This lint would have been useful in finding #31594 before it was merged, rather than only after the canary deploy.

This PR also fixes a bunch of other likely wrong usages of const (all only performance-impacting afaict), as well as a bunch of other clippy errors one gets on the recent Rust version.

Motivation

  • This PR refactors existing code.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

now.saturating_add(
TIMESTAMP_INTERVAL.saturating_mul(Timestamp::from(TIMESTAMP_INTERVAL_UPPER_BOUND)),
)
now.saturating_add(TIMESTAMP_INTERVAL_MS * TIMESTAMP_INTERVAL_UPPER_BOUND)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk what the meaning of the two constants here ("timestamp interval" and "timestamp interval upper bound") is, but it seems like maybe it could be a single constant?

/// Set of [`SystemVar`]s that can also get set at a per-Session level.
///
/// TODO(parkmycar): Instead of a separate list, make this a field on VarDefinition.
const SESSION_VARS: LazyLock<BTreeMap<&'static UncasedStr, &'static VarDefinition>> =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move this outside the SystemVars impl because associated static items are not allowed.

Mostly about unused lifetime specifiers in function signatures.
@teskje teskje force-pushed the clippy-borrow_interior_mutable_const branch from 661ce08 to 77f54b8 Compare March 12, 2025 15:58
@teskje teskje force-pushed the clippy-borrow_interior_mutable_const branch from 77f54b8 to eb687f8 Compare March 12, 2025 16:23
@teskje teskje marked this pull request as ready for review March 12, 2025 16:35
@teskje teskje requested review from a team as code owners March 12, 2025 16:35
@teskje teskje requested review from aljoscha and antiguru March 12, 2025 16:35
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

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

Yes! Thank you

@teskje
Copy link
Contributor Author

teskje commented Mar 13, 2025

TFTR!

@teskje teskje merged commit e5475a3 into MaterializeInc:main Mar 13, 2025
83 checks passed
@teskje teskje deleted the clippy-borrow_interior_mutable_const branch March 13, 2025 09:25
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.

2 participants