Apply mismatched-lifetime-syntaxes to trait and extern functions

[shepmaster]

r? @jieyouxu

[rustbot]

jieyouxu is not on the review rotation at the moment.
They may take a while to respond.

[jieyouxu]

Waiting a bit for discussions in #t-lang > Feedback on mismatched_lifetime_syntaxes. I'll come back to this approx. next Wednesday.

[jieyouxu]

@rustbot note check-impact-for-svd2rust

(Reminder for myself) check impact of expanding the lint against https://github.com/rust-embedded/svd2rust -> https://github.com/stm32-rs/stm32-rs.

[jieyouxu]

@rustbot note perf-before-merge

(Reminder for myself) check perf before merge, even tho again I expect this to be dominated by warning emission cost.

[jieyouxu]

Thanks. The implementation and tests look good to me. We should:

1. Check the perf impact.
2. Check if svd2rust is again impacted (if they use trait generators that exhibit the problem, it's possible that the fix of the generator is quite trivial but may impact generated crates like stm32f4 a lot for local dev builds).

[jieyouxu]

@bors2 try @rust-timer queue

[rust-bors]

⌛ Trying commit 9c6320a with merge 39a127b…

To cancel the try build, run the command @bors2 try cancel.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 39a127b (39a127b7add9461c84cd14b9c8cf249824b0c9c4, parent: 0a39445252ac076ef573bcacee63bbdc59497b52)

[rust-timer]

Finished benchmarking commit (39a127b): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.3%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.3%, secondary 4.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[1.7%, 3.1%]	3
Regressions ❌ (secondary)	4.8%	[0.9%, 8.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[1.7%, 3.1%]	3

Cycles

Results (secondary 0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 754.356s -> 753.447s (-0.12%)
Artifact size: 372.07 MiB -> 372.14 MiB (0.02%)

[jieyouxu]

Looked at the perf results. ctfe-stress-5 and coercions are unrelated noise. externs (check, {full,incr-full}) regressions look genuine (albeit minimal), but it looks like we're just necessarily doing more work on the synthetic benchmark with 3000 copies of extern fns (not from lint emissions, as no warnings are emitted).

@rustbot note remove perf-before-merge

[jieyouxu]

I looked at stm32-rs (whose crates are generated with svd2rust). In particular, I looked at stm32f4 as a cherry-picked example who is generated by svd2rust. I checked out https://github.com/stm32-rs/stm32-rs-nightlies which has generated nightly versions of stm32f4 locally, and observed the output with a stage1 toolchain that has this lint expansion.

$ cargo +stage1 check --features=stm32f405

Looks like this will cause quite a few new warnings. Very rough examples (they exhibit the same pattern):

can1/mcr.rs
	pub fn inrq(&mut self) -> INRQ_W<MCRrs>
cryp/cr.rs
hash/csr.rs
hash/str.rs
hash/sr.rs
hash/imr.rs
cryp/din.rs
cryp/dmacr.rs
cryp/imscr.rs
cryp/key/klr.rs
cryp/key/krr.rs
cryp/init/ivlr.rs
cryp/init/ivrr.rs
	pub fn iv(&mut self) -> IV_W<IVRRrs>
cryp/csgcmccmr.rs
	pub fn csgcmccmr(&mut self) -> CSGCMCCMR_W<CSGCMCCMRrs>
cryp/csgcmr.rs
	pub fn csgcmr(&mut self) -> CSGCMR_W<CSGCMRrs>
[...]

Looks like the way the embedded crates are setup involves quite a few traits which will then trigger this lint expansion.

I might look into svd2rust (as that's used by quite a few embedded crates) to see if we can preempt the warnings, but I can only get to that at the earliest next Monday.

[shepmaster]

I looked at stm32-rs (whose crates are generated with svd2rust)

I might look into svd2rust (as that's used by quite a few embedded crates) to see if we can preempt the warnings

I tried to jump ahead here, but it's always possible I made a misstep:

1. Cloned the newest version of stm32-rs.
2. Followed their instructions to setup a local build, except I installed the newest, unreleased version of svd2rust. This has changes for the relevant lint.
3. Ran cargo +stage1 check --features=stm32f405 in the stm32f4 directory, and it was clean.

I think the code you tested is using the released svd2rust without the changes.

involves quite a few traits which will then trigger this lint expansion

Looking at the generated stm32f4 directory, I see very few traits, actually...

% rg -F 'trait' --no-ignore stm32f4
stm32f4/src/generic.rs
50:pub trait RawReg:
93:pub trait RegisterSpec {
99:pub trait FieldSpec: Sized {
105:pub trait IsEnum: FieldSpec {}
110:pub trait Readable: RegisterSpec {}
117:pub trait Writable: RegisterSpec {
132:pub trait Resettable: RegisterSpec {
961:    pub trait AtomicOperations {

Many of these only have associated constants or types, and the few that have methods look unsuspicious to me.

[jieyouxu]

Ah sorry, I think you're right. I'll revisit this.

[jieyouxu]

I think you're right, thanks for the correction.
@rustbot note remove check-impact-for-svd2rust

[jieyouxu]

The impl otherwise looks good. Do you think we should update the lint docs to also add two examples, one for a trait method, and another for extern fn?

[shepmaster]

update the lint docs

Tl;dr: no.

My first thought is that checking these functions would be the “expected” behavior of the lint, so calling them out in the docs would be more confusing and showing more of an implementation detail than a functional change.

[jieyouxu]

Yeah, I was thinking about that, and it seemed like it might be distracting even though it might show more examples.

Seems fine then. Thanks!

[bors]

📌 Commit 9c6320a has been approved by jieyouxu

It is now in the queue for this repository.

[jieyouxu]

I'll tag this will relnotes, but then explicitly close it and backlink that relnotes to the relnotes for the "base" lint PR.

@rustbot label: +relnotes

[bors]

⌛ Testing commit 9c6320a with merge 4a73e3c...

[bors]

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 4a73e3c to master...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 9822e3d (parent) -> 4a73e3c (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 4a73e3c224465c0c0e71b39b479a0911460dd794 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-apple-various: 5303.5s -> 7184.5s (35.5%)
2. dist-x86_64-mingw: 7274.5s -> 9513.5s (30.8%)
3. dist-i686-mingw: 7725.3s -> 9808.6s (27.0%)
4. x86_64-mingw-2: 7219.4s -> 8962.6s (24.1%)
5. x86_64-apple-2: 4842.5s -> 3721.0s (-23.2%)
6. x86_64-mingw-1: 8854.1s -> 10904.6s (23.2%)
7. x86_64-apple-1: 8479.5s -> 6603.2s (-22.1%)
8. x86_64-msvc-2: 8059.7s -> 6725.8s (-16.6%)
9. mingw-check-1: 1896.8s -> 1620.5s (-14.6%)
10. mingw-check-tidy: 83.8s -> 72.3s (-13.8%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (4a73e3c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 1.0%, secondary -1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.5%	[-5.8%, -3.2%]	2
All ❌✅ (primary)	1.0%	[1.0%, 1.0%]	1

Cycles

Results (secondary 4.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 757.645s -> 757.535s (-0.01%)
Artifact size: 372.28 MiB -> 372.26 MiB (-0.00%)