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

Uncertain maybe-incorrect suggestion for impl_trait_overcaptures #132932

Closed
ehuss opened this issue Nov 12, 2024 · 7 comments
Closed

Uncertain maybe-incorrect suggestion for impl_trait_overcaptures #132932

ehuss opened this issue Nov 12, 2024 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-edition Diagnostics: An error or lint that should account for edition differences. L-impl_trait_overcaptures Lint: impl_trait_overcaptures T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented Nov 12, 2024

This is mostly a question for @compiler-errors (and really just an edition-guide issue). This is regarding the maybe-incorrect suggestion for impl_trait_overcaptures in the following specific scenario:

https://crater-reports.s3.amazonaws.com/pr-132712/try%23da25749bf5e6ba5ed862ff361c19afff2a986b2d/reg/srdf-0.1.52/log.txt

where the general code looks like:

impl<RDF> RDFParser<RDF>
where
    RDF: FocusRDF,
{
   //................
    pub fn instances_of(
        &self,
        object: &RDF::Term,
    ) -> Result<impl Iterator<Item = RDF::Subject>, RDFParseError> {
        let values = self
            .rdf
            .subjects_with_predicate_object(&Self::rdf_type(), object)
            .map_err(|e| RDFParseError::SRDFError { err: e.to_string() })?;
        Ok(values.into_iter())
    }
}

This generates the impl_trait_overcaptures with maybe-incorrect applicability.

warning: `impl Iterator<Item = <RDF as srdf_basic::SRDFBasic>::Subject>` will capture more lifetimes than possibly intended in edition 2024
  --> src/srdf_parser/rdf_parser.rs:65:17
   |
65 |     ) -> Result<impl Iterator<Item = RDF::Subject>, RDFParseError> {
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = warning: this changes meaning in Rust 2024
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/rpit-lifetime-capture.html>
note: specifically, these lifetimes are in scope but not mentioned in the type's bounds
  --> src/srdf_parser/rdf_parser.rs:63:9
   |
63 |         &self,
   |         ^
64 |         object: &RDF::Term,
   |                 ^
   = note: all lifetimes in scope will be captured by `impl Trait`s in edition 2024
help: use the precise capturing `use<...>` syntax to make the captures explicit
   |
65 |     ) -> Result<impl Iterator<Item = RDF::Subject> + use<RDF>, RDFParseError> {

The question I have is that I do not see this scenario described in https://doc.rust-lang.org/nightly/edition-guide/rust-2024/rpit-lifetime-capture.html, and thus left being uncertain about why it thinks it is maybe incorrect. I assume it is something to do with the associated types, but it wasn't immediately obvious how to minimize this scenario.

If this isn't already documented on https://doc.rust-lang.org/nightly/edition-guide/rust-2024/rpit-lifetime-capture.html about why it might be wrong to blindly follow the compiler, would it be possible to describe the scenario and what considerations the author should take before accepting the suggestion?

Meta

rustc --version --verbose:

rustc 1.84.0-nightly (143ce0920 2024-11-10)
binary: rustc
commit-hash: 143ce0920a2307b19831160a01f06f107610f1b2
commit-date: 2024-11-10
host: aarch64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3
@ehuss ehuss added A-edition-2024 Area: The 2024 edition C-bug Category: This is a bug. D-edition Diagnostics: An error or lint that should account for edition differences. L-impl_trait_overcaptures Lint: impl_trait_overcaptures labels Nov 12, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 12, 2024
@compiler-errors
Copy link
Member

I can't remember why I marked it as maybe-incorrect, though in this case we probably will have a problem since it'll suggest + use<'_> which is ambiguous in this case.

I don't have the time to majorly overhaul how we issue this suggestion to turn that into a 'a or something.

I think we can probably make it machine applicable except for when there's a '_ lifetime in the suggestion.

@compiler-errors
Copy link
Member

Another place where it cannot be machine applicable is when there are APITs, since turning a impl Trait argument into a T: Trait changes the API of the function (i.e. makes it turbofishable).

@ehuss
Copy link
Contributor Author

ehuss commented Nov 12, 2024

The suggestions it gives in every case is + use<RDF> which seems to at least compile (except one function, which I don't know how to fix). That's partially why I'm a little confused, because it doesn't seem to be related to any named or unnamed lifetimes and it's not an APIT, either.

Regarding the case with the ambiguous lifetime, I don't think I have seen that in the crater run, yet.

And I'm now seeing that #132816 changed it so that all impl_trait_overcaptures suggestions are maybe-incorrect. Am I interpreting that correctly? The PR doesn't mention that change.

That could be a significant hurdle for some projects since some will have dozens of RPITs to update.

If we are going with all maybe-incorrect, we'll at least need to update the documentation in https://doc.rust-lang.org/nightly/edition-guide/rust-2024/rpit-lifetime-capture.html#migrating-while-avoiding-overcapturing that you'll need to make all changes manually.

@compiler-errors
Copy link
Member

Am I interpreting that correctly? The PR doesn't mention that change.

Yes, and that was an oversight. Sorry, I'm not perfect :)

@compiler-errors
Copy link
Member

Also, I misspoke. There should never be a case where we capture an elided lifetime that would be incorrect to name in the + use<'_> suggestion. It should only be in the case that we have APITs.

@compiler-errors
Copy link
Member

#132938 addresses this

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 12, 2024
…r=chenyukang

Make precise capturing suggestion machine-applicable only if it has no APITs

cc rust-lang#132932

The only case where this suggestion is not machine-applicable is when we suggest turning arg-position impl trait into type parameters, which may expose type parameters that were not turbofishable before.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 12, 2024
Rollup merge of rust-lang#132938 - compiler-errors:ed2024-apit-sugg, r=chenyukang

Make precise capturing suggestion machine-applicable only if it has no APITs

cc rust-lang#132932

The only case where this suggestion is not machine-applicable is when we suggest turning arg-position impl trait into type parameters, which may expose type parameters that were not turbofishable before.
@ehuss
Copy link
Contributor Author

ehuss commented Nov 13, 2024

Closing as I believe this is now resolved. Thanks!

@ehuss ehuss closed this as completed Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-edition Diagnostics: An error or lint that should account for edition differences. L-impl_trait_overcaptures Lint: impl_trait_overcaptures T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants