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

bump MSRV to 1.83 #16294

Merged
merged 1 commit into from
Feb 26, 2025
Merged

bump MSRV to 1.83 #16294

merged 1 commit into from
Feb 26, 2025

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Feb 21, 2025

According to our new MSRV policy (see #16370 ), bump our MSRV to 1.83 (N - 2), and autofix some new clippy lints.

Copy link
Contributor

github-actions bot commented Feb 21, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser added breaking Breaking API change and removed breaking Breaking API change labels Feb 21, 2025
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

This is technically a breaking change for anyone building Ruff from source because they now need a newer Rust version to do so.

I have a slight preference to keep the current MSRV unless there's a lot of boilerplate involved.

The other thing we have to consider when bumping the MSRV is whether downstream packaging tools have updated to this Rust version (e.g. homebrew). This should be fine, considering that uv already uses 1.83

@carljm
Copy link
Contributor Author

carljm commented Feb 21, 2025

Ok I'll try the boilerplate tomorrow. Just annoying writing uglier code that shouldn't need to be that way to accommodate older rust versions :/ doesn't seem like it should be hard for anyone to use 1.83 instead of 1.80, and I'm sure we'll bump it up sometime...

@MichaReiser
Copy link
Member

I'd say it depends on how much more boilerplate it requires. If it's something you can do in 15min, let's keep the MSRV as is. If not, bump it (although a lower MSRV might also be important for other projects using salsa)

@carljm
Copy link
Contributor Author

carljm commented Feb 22, 2025

If it's something you can do in 15min

I wasn't able to find something that compiles within 15min, but maybe you know how to do it.

The problem is at https://github.com/salsa-rs/salsa/blob/1ff42613cbaa699567009895a6d00b87bf7772e6/src/cycle.rs#L97 -- hashset iterators only starting implementing Default in Rust 1.83.

In the owning IntoIterator impl above I can just replace .unwrap_or_default() with .unwrap_or_else(|| FxHashSet::default().into_iter()). But in the borrowed version I can't figure out what I can put in the .unwrap_or_else() without getting errors about borrowing from a local, or returning the wrong type. Even if I remove the Copied and let it be just an iterator over &DatabaseKeyIndex, I still get errors about borrowing from a local if I try to use FxHashSet::default().iter().

@MichaReiser
Copy link
Member

Yeah, I think you'd have to write your own iterator. Which is definitely less nice but it's not a show-stopper:

impl std::iter::IntoIterator for CycleHeads {
    type Item = DatabaseKeyIndex;
    type IntoIter = std::collections::hash_set::IntoIter<Self::Item>;

    fn into_iter(self) -> Self::IntoIter {
        self.0
            .map(|heads| *heads)
            .unwrap_or_else(|| FxHashSet::default())
            .into_iter()
    }
}

pub struct CycleHeadsIter<'a>(Option<std::collections::hash_set::Iter<'a, DatabaseKeyIndex>>);

impl<'a> Iterator for CycleHeadsIter<'a> {
    type Item = DatabaseKeyIndex;

    fn next(&mut self) -> Option<Self::Item> {
        self.0.as_mut()?.next().copied()
    }

    fn last(self) -> Option<Self::Item> {
        self.0?.last().copied()
    }
}

impl FusedIterator for CycleHeadsIter<'_> {}

impl<'a> std::iter::IntoIterator for &'a CycleHeads {
    type Item = DatabaseKeyIndex;
    type IntoIter = CycleHeadsIter<'a>;

    fn into_iter(self) -> Self::IntoIter {
        CycleHeadsIter(self.0.as_ref().map(|heads| heads.iter()))
    }
}

@carljm
Copy link
Contributor Author

carljm commented Feb 22, 2025

Thanks! Added this to the Salsa PR to maintain 1.80 compat, will close this for now.

@carljm carljm closed this Feb 22, 2025
@MichaReiser MichaReiser added the internal An internal refactor or improvement label Feb 26, 2025
@carljm carljm merged commit dd6f623 into main Feb 26, 2025
21 checks passed
@carljm carljm deleted the cjm/bump-msrv branch February 26, 2025 14:12
dcreager added a commit that referenced this pull request Feb 27, 2025
* main:
  [red-knot] unify LoopState and saved_break_states (#16406)
  [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378)
  [FURB156] Do not consider docstring(s) (#16391)
  Use `is_none_or` in `stdlib-module-shadowing` (#16402)
  [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398)
  [red-knot] Fix file watching for new non-project files (#16395)
  document MSRV policy (#16384)
  [red-knot] fix non-callable reporting for unions (#16387)
  bump MSRV to 1.83 (#16294)
  Avoid unnecessary info at non-trace server log level (#16389)
  Expand `ruff.configuration` to allow inline config (#16296)
  Start detecting version-related syntax errors in the parser (#16090)
dcreager added a commit that referenced this pull request Feb 27, 2025
* dcreager/dont-have-a-cow:
  [red-knot] unify LoopState and saved_break_states (#16406)
  [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378)
  [FURB156] Do not consider docstring(s) (#16391)
  Use `is_none_or` in `stdlib-module-shadowing` (#16402)
  [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398)
  [red-knot] Fix file watching for new non-project files (#16395)
  document MSRV policy (#16384)
  [red-knot] fix non-callable reporting for unions (#16387)
  bump MSRV to 1.83 (#16294)
  Avoid unnecessary info at non-trace server log level (#16389)
  Expand `ruff.configuration` to allow inline config (#16296)
  Start detecting version-related syntax errors in the parser (#16090)
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