Skip to content

chore(all): Migrate all instances of #[allow] to #[expect]#1561

Merged
ethanoroshiba merged 8 commits intomainfrom
ENG-819/allow_to_expect
Sep 30, 2024
Merged

chore(all): Migrate all instances of #[allow] to #[expect]#1561
ethanoroshiba merged 8 commits intomainfrom
ENG-819/allow_to_expect

Conversation

@ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Sep 25, 2024 •
edited
Loading

Summary

Implemented usage of #[expect] instead of #[allow] following bump to Rust 1.81

Background

With the recent bump to Rust 1.81 (#1523), the usage of #[expect] is now stable. This is preferable because it warns in case the lint is unfulfilled, keeping us from having superfluous allow attributes.

Changes

  • Changed test.yml to include clippy::allow_attributes and clippy::allow_attributes_with_no_reason lints
  • Updated all crates to use Rust 1.81.0
  • Migrated all instances of #[allow] to #[expect], with the exception of our generated modules.
  • Deleted all allow attributes which were no longer useful

Testing

Passing all tests

Related Issues

closes #1521

@github-actions github-actions bot added ci issues that are related to ci and github workflows conductor pertaining to the astria-conductor crate sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels Sep 25, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review September 25, 2024 18:55
@ethanoroshiba ethanoroshiba requested review from a team as code owners September 25, 2024 18:55
@SuperFluffy
Copy link
Contributor

SuperFluffy commented Sep 26, 2024

It seems like this does not work as expected if msrv is set below 1.81: rust-lang/rust-clippy#13348

Because this change is pervasive and expected to be used in every single crate of ours, can you bump the msrv of all crates to 1.81? We should hopefully see clippy triggering).

SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
crates/astria-merkle/benches/benchmark.rs Outdated
Comment on lines 10 to 17
#[expect(
clippy::allow_attributes,
reason = "this allow is conditional and hence necessary"
)]
#[allow(
dead_code,
reason = "unused warning if `bench_include_allocs` feature is not enabled"
)]
Copy link
Contributor

@SuperFluffy SuperFluffy Sep 27, 2024

Choose a reason for hiding this comment

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

Can't you also tuck this under the cfg_attr below?

Copy link
Contributor Author

@ethanoroshiba ethanoroshiba Sep 27, 2024

Choose a reason for hiding this comment

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

Replaced with #cfg(feature = "bench_include_allocs")

SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
crates/astria-sequencer/src/app/test_utils.rs
Copy link
Contributor

@SuperFluffy SuperFluffy Sep 27, 2024

Choose a reason for hiding this comment

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

Comment on all the expect lines that go above a cfg_attr attribute: I think you should be able to put them under the attribute, something like this:

#[cfg_attr(
    feature = "bla",
    expect(dead_code, reason = "benchmarks use only some parts of the test code").,
)]

Copy link
Contributor Author

@ethanoroshiba ethanoroshiba Sep 27, 2024

Choose a reason for hiding this comment

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

As discussed offline, this unfortunately doesn't work because clippy evaluates the expect attribute even when the feature is not enabled. Tracking issue here: #1585

SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy reviewed Sep 27, 2024
View reviewed changes
SuperFluffy
SuperFluffy approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for going through all of those.

I have left a number of comments where I think we should follow up with issues, or where the allow and/or expect are so wordy, that we should actually just get rid of them in favor of a simpler fix.

Also, there are a few instances where you had expect on top of cfg_attr attributes, which I think could be removed, and instead replacing the value of the cfg_attr by expect.

@ethanoroshiba ethanoroshiba mentioned this pull request Sep 27, 2024
Closed
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Sep 30, 2024
Merged via the queue into main with commit eced579 Sep 30, 2024
@ethanoroshiba ethanoroshiba deleted the ENG-819/allow_to_expect branch September 30, 2024 15:18
@ethanoroshiba ethanoroshiba mentioned this pull request Sep 30, 2024
Merged
@SuperFluffy SuperFluffy mentioned this pull request Sep 30, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 2, 2024
@ethanoroshiba
chore(sequencer): simplify boolean expressions in `transaction contai…
6fdf4ea 
…ner` (#1595)

## Summary
Simplified logical statements in
`transaction_priority_comparisons_should_be_consistent_nonce_diff()`.

## Background
Previously there was an allow for `clippy::nonminimal_bool`. The
reasoning behind it was to match documented behavior. This change is
meant to explicitly state the expected behavior while still simplifying
the boolean expressions. This is in response to this comment:
#1561 (comment)

## Changes
- Simplified boolean expressions and moved the non-simplified versions
to the comments where applicable to provide context on the documented
behavior.

## Testing
Passing all tests

## Related Issues
closes #1583
steezeburger added a commit that referenced this pull request Oct 7, 2024
@steezeburger
Merge branch 'main' into spike/noble-usdc
df709c6 
* main: (34 commits)
  feat(proto): add bundle and optimistic block apis (#1519)
  feat(sequencer)!: make empty transactions invalid  (#1609)
  chore(sequencer): simplify boolean expressions in `transaction container` (#1595)
  refactor(cli): merge argument parsing and command execution (#1568)
  feat(charts): astrotrek chart (#1513)
  chore(charts): genesis template to support latest changes (#1594)
  fix(ci): code freeze action fix (#1610)
  chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492)
  ci: code freeze through github actions (#1588)
  refactor(sequencer): use builder pattern for transaction container tests (#1592)
  feat(conductor)!: implement chain ID checks (#1482)
  chore(ci): upgrade audit-check (#1577)
  feat(sequencer)!: transaction categories on UnsignedTransaction (#1512)
  fix(charts): sequencer prometheus rules   (#1598)
  chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561)
  chore(charts,sequencer-faucet): asset precision (#1517)
  chore(docs): endpoints (#1543)
  fix(docker): use target binary build param as name of image entrypoint (#1573)
  fix(ci): ibc bridge test timeout (#1587)
  Feature: Add `graph-node` to charts. (#1489)
  ...
sgranfield4403-3 added a commit to sgranfield4403-3/astria that referenced this pull request Oct 2, 2025
@sgranfield4403-3
chore(sequencer): simplify boolean expressions in `transaction contai…
0998a46 
…ner` (#1595)

## Summary
Simplified logical statements in
`transaction_priority_comparisons_should_be_consistent_nonce_diff()`.

## Background
Previously there was an allow for `clippy::nonminimal_bool`. The
reasoning behind it was to match documented behavior. This change is
meant to explicitly state the expected behavior while still simplifying
the boolean expressions. This is in response to this comment:
astriaorg/astria#1561 (comment)

## Changes
- Simplified boolean expressions and moved the non-simplified versions
to the comments where applicable to provide context on the documented
behavior.

## Testing
Passing all tests

## Related Issues
closes #1583
AngieD101 added a commit to AngieD101/astria that referenced this pull request Oct 10, 2025
@AngieD101
chore(sequencer): simplify boolean expressions in `transaction contai…
83d7247 
…ner` (#1595)

## Summary
Simplified logical statements in
`transaction_priority_comparisons_should_be_consistent_nonce_diff()`.

## Background
Previously there was an allow for `clippy::nonminimal_bool`. The
reasoning behind it was to match documented behavior. This change is
meant to explicitly state the expected behavior while still simplifying
the boolean expressions. This is in response to this comment:
astriaorg/astria#1561 (comment)

## Changes
- Simplified boolean expressions and moved the non-simplified versions
to the comments where applicable to provide context on the documented
behavior.

## Testing
Passing all tests

## Related Issues
closes #1583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WafflesVonMaple WafflesVonMaple Awaiting requested review from WafflesVonMaple WafflesVonMaple was automatically assigned from astriaorg/infra

2 more reviewers

@SuperFluffy SuperFluffy SuperFluffy approved these changes

@quasystaty1 quasystaty1 quasystaty1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ci issues that are related to ci and github workflows code-quality composer pertaining to composer conductor pertaining to the astria-conductor crate sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace all allow attributes by expect and require lint reasons

3 participants

@ethanoroshiba @SuperFluffy @quasystaty1