Comments (12)
I think I have reduced the issue to this MCVE:
#![allow(dead_code, non_camel_case_types)]
trait JoinTo {}
fn simple_belongs_to() {
mod posts {
pub struct table {}
}
impl JoinTo for posts::table {}
}
from rust.
@rustbot labels -I-lang-nominated
We discussed this in the lang meeting today. Our consensus was in favor of adopting the modification to the RFC 3373 rules discussed here, to make "modules transparent when checking if the impl
def is at the same nesting as the" type or trait.
I.e., this code is not a "sneaky inner impl" and should not lint:
trait Trait {}
fn foo() {
mod m {
pub struct Ty {}
}
impl Trait for m::Ty {}
}
This is good to go on the lang side, and we leave it as usual to the compiler team to decide on the appropriate follow-up actions.
from rust.
A gap in the implementation of #122747? The check probably doesn't account for this case:
rust/compiler/rustc_lint/src/non_local_def.rs
Lines 167 to 169 in 6acb9e7
from rust.
The RFC says that either the Trait
or the Type
should at the same nesting as the impl
block, in the implementation we look at the parent item to see if they are equal.
In the MCVE:
- the
impl
block parent isfn simple_belongs_to
JoinTo
trait parent is the crate root- and
posts::table
parent ismod posts
None of those are equal, so we emit the lint.
However, this is were things get interesting, rust-analyzer actually also consider "all ancestor of the current block"; which means that the impl
block would be correctly taken into consideration, because in r-a terms block are not modules, just expressions; so making modules transparent would fix the issue in a way that is compatible with r-a.
edit: to be ignored
Except that it would fall apart quickly for r-a as not all parent blocks are not taken into consideration. EDIT: I was wrong, I was missing a import.
trait JoinTo {
fn gg(&self) {}
}
fn simple_belongs_to() {
mod posts {
pub mod posts {
pub struct table;
}
fn hh() {
use crate::JoinTo; // EDIT: actually `rustc` doesn't compile without this import
// and adding it makes r-a also resolve this 🤔
let _a = posts::table.gg(); // --this is NOT resolved by r-a--
}
}
impl JoinTo for posts::posts::table {}
let _a = posts::posts::table.gg(); // but here it is resolved by r-a
}
Therefore strictly speaking I don't think is a false positive.
from rust.
Thanks to @Veykril, I now better understand r-a internals. I have confirmed with him that making modules transparent when checking if the impl
def is at the same nesting as the Type
or Trait
would be fine for r-a1.
Implementation wise making modules transparent would be very similar to the const _: () = { ... }
exception that we already have. In the sense that we would consider all parent modules of the Type
/Trait
to be transparent, no neighbours.
I tried finding examples where type inference would make the impl non-local but couldn't find any, that doesn't mean there aren't, just that if there are, I was not able to find any.
Whenever we should do this is beyond me. cc @joshtriplett
Footnotes
-
Internally, when doing type/method resolutions, r-a considers, among other things, all ancestors of the current block, in which modules are basically transparent. ↩
from rust.
I don't see any obvious ways to get non-local impls out of this either. Assuming nobody finds any, this exception (treating a local module's contents as local) seems reasonable and consistent with what we're trying to do here.
I also don't see any obvious ways in which this closes off future avenues for adding ways to reach inside/outside of a module-inside-function. We've talked about adding a way for a nested module to reference the containing fn's scope, but that wouldn't create non-local behavior.
I'm nominating this for lang. If anyone in lang can see a way to get a non-local impl out of a module-inside-fn like this, please speak up. Otherwise, I think it's reasonable to fix this as proposed.
from rust.
There's another false positive:
struct A;
fn foo() {
struct B;
impl From<B> for A {
fn from(b: B) -> A {
todo!()
}
}
}
The error shown is:
warning: non-local `impl` definition, they should be avoided as they go against expectation
--> src/lib.rs:6:5
|
6 | / impl From<B> for A {
7 | | fn from(b: B) -> A {
8 | | todo!()
9 | | }
10 | | }
| |_____^
|
= help: move this `impl` block outside the of the current function `foo`
= note: an `impl` definition is non-local if it is nested inside an item and may impact type checking outside of that item. This can be the case if neither the trait or the self type are at the same nesting level as the `impl`
= note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module and anon-const at the same nesting as the trait or type
= note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>
= note: `#[warn(non_local_definitions)]` on by default
Should I file a new issue?
from rust.
Thanks you all for your swift response. It would be great to see that resolved before it hits stable. The lint seems now to be enabled on the beta branch as well.
Also I want to highlight the other part of the issue (misleading message) again:
Especially the suggestion
= note: the macro allow_tables_to_appear_in_same_query may come from an old version of the diesel crate, try updating your dependency with cargo update -p diesel
is highly problematic in this context as that never could resolve the error given above. The wording might result in cases where the user will believe that this is an upstream diesel issue, rather than an issue with their local code.
I feel that this should detect whether an update could even resolve this issue. This is not the case if the user put the macro call inside a function on it's own, which I feel is something that should be easy to detect while emitting the lint. (If someone points to the right location I even can have a look at that myself.)
from rust.
Updating the labels so we track this issue as a regression.
@rustbot labels -needs-triage +regression-from-stable-to-beta
from rust.
WG-prioritization assigning priority (Zulip discussion).
Since this looks like a diag regression so maybe confusing but not critical.
Fixed by #124539 (beta-nominated)
@rustbot label -I-prioritize +P-medium
from rust.
I think since this lint is new in 1.79 this is a higher priority than a normal diagnostic regression. Retagging as P-high.
@rustbot label -P-medium +P-high
from rust.
The fixed has landed in 1.80 (#124539) and now in 1.79 (#124947) as well.
from rust.
Related Issues (20)
- APITIT get treated as ordinary params in "not all trait impls implemented" error HOT 1
- `core::ptr::copy` unsafe precondition message is for `core::ptr::copy_nonoverlapping` instead
- Potential overflow in the calculation of complex trait bounds involving tuples HOT 1
- TAIT: type parameter `impl FnOnce()` is part of concrete type but not used in parameter list for the `impl Trait` type alias HOT 4
- Weird diagnostic for recursion in const fn HOT 1
- Struct update syntax doesn't reborrow mut references HOT 4
- panic during codegen/LLVM phase HOT 2
- ICE: `relating different kinds` HOT 3
- ICE: `Tried to make Ignore indirect` HOT 1
- Bad codegen using `u16::to/from_be_bytes` HOT 1
- `struct` documentation should include `Sized` in the listed auto trait implementations HOT 2
- Bootstrap is slow HOT 8
- UpperHex formatting might panic HOT 3
- Repeated spurious rustdoc test failure: rustdoc-gui/help-page HOT 2
- Panic when setting RUST_MIN_STACK incorrectly HOT 5
- Docs produce invalid type when using bounds in associated type position HOT 1
- `unnameable_types` should show where the type is reachable HOT 2
- MIR_borrow_checking and MIR_effect_checking are very slow HOT 2
- Bevy segfaults on some Windows systems after Rust 1.79 update HOT 24
- ICE with generic_const_exprs (and std::simd)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust.