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

Cleanup: Move an impl-Trait check from AST validation to AST lowering #132214

Merged
merged 1 commit into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions compiler/rustc_ast_lowering/src/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
modifiers: Option<ast::TraitBoundModifiers>,
) -> hir::QPath<'hir> {
let qself_position = qself.as_ref().map(|q| q.position);
let qself = qself.as_ref().map(|q| self.lower_ty(&q.ty, itctx));
let qself = qself
.as_ref()
// Reject cases like `<impl Trait>::Assoc` and `<impl Trait as Trait>::Assoc`.
.map(|q| self.lower_ty(&q.ty, ImplTraitContext::Disallowed(ImplTraitPosition::Path)));

let partial_res =
self.resolver.get_partial_res(id).unwrap_or_else(|| PartialRes::new(Res::Err));
Expand Down Expand Up @@ -75,6 +78,16 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
None
};

// Only permit `impl Trait` in the final segment. E.g., we permit `Option<impl Trait>`,
// `option::Option<T>::Xyz<impl Trait>` and reject `option::Option<impl Trait>::Xyz`.
let itctx = |i| {
if i + 1 == p.segments.len() {
itctx
} else {
ImplTraitContext::Disallowed(ImplTraitPosition::Path)
}
};

let path_span_lo = p.span.shrink_to_lo();
let proj_start = p.segments.len() - unresolved_segments;
let path = self.arena.alloc(hir::Path {
Expand Down Expand Up @@ -121,7 +134,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
segment,
param_mode,
generic_args_mode,
itctx,
itctx(i),
bound_modifier_allowed_features.clone(),
)
},
Expand Down Expand Up @@ -185,7 +198,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
segment,
param_mode,
generic_args_mode,
itctx,
itctx(i),
None,
));
let qpath = hir::QPath::TypeRelative(ty, hir_segment);
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_ast_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ ast_passes_generic_before_constraints = generic arguments must come before the f

ast_passes_generic_default_trailing = generic parameters with a default must be trailing

ast_passes_impl_trait_path = `impl Trait` is not allowed in path parameters

ast_passes_incompatible_features = `{$f1}` and `{$f2}` are incompatible, using them at the same time is not allowed
.help = remove one of these features

Expand Down
46 changes: 0 additions & 46 deletions compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ struct AstValidator<'a> {

disallow_tilde_const: Option<TildeConstReason>,

/// Used to ban `impl Trait` in path projections like `<impl Iterator>::Item`
/// or `Foo::Bar<impl Trait>`
is_impl_trait_banned: bool,

/// Used to ban explicit safety on foreign items when the extern block is not marked as unsafe.
extern_mod_safety: Option<Safety>,

Expand Down Expand Up @@ -123,12 +119,6 @@ impl<'a> AstValidator<'a> {
self.extern_mod_safety = old;
}

fn with_banned_impl_trait(&mut self, f: impl FnOnce(&mut Self)) {
let old = mem::replace(&mut self.is_impl_trait_banned, true);
f(self);
self.is_impl_trait_banned = old;
}

fn with_tilde_const(
&mut self,
disallowed: Option<TildeConstReason>,
Expand Down Expand Up @@ -213,37 +203,6 @@ impl<'a> AstValidator<'a> {
.with_tilde_const(Some(TildeConstReason::TraitObject), |this| {
visit::walk_ty(this, t)
}),
TyKind::Path(qself, path) => {
// We allow these:
// - `Option<impl Trait>`
// - `option::Option<impl Trait>`
// - `option::Option<T>::Foo<impl Trait>`
//
// But not these:
// - `<impl Trait>::Foo`
// - `option::Option<impl Trait>::Foo`.
//
// To implement this, we disallow `impl Trait` from `qself`
// (for cases like `<impl Trait>::Foo>`)
// but we allow `impl Trait` in `GenericArgs`
// iff there are no more PathSegments.
if let Some(qself) = qself {
// `impl Trait` in `qself` is always illegal
self.with_banned_impl_trait(|this| this.visit_ty(&qself.ty));
}

// Note that there should be a call to visit_path here,
// so if any logic is added to process `Path`s a call to it should be
// added both in visit_path and here. This code mirrors visit::walk_path.
for (i, segment) in path.segments.iter().enumerate() {
// Allow `impl Trait` iff we're on the final path segment
if i == path.segments.len() - 1 {
self.visit_path_segment(segment);
} else {
self.with_banned_impl_trait(|this| this.visit_path_segment(segment));
}
}
}
_ => visit::walk_ty(self, t),
}
}
Expand Down Expand Up @@ -737,10 +696,6 @@ impl<'a> AstValidator<'a> {
}
}
TyKind::ImplTrait(_, bounds) => {
if self.is_impl_trait_banned {
self.dcx().emit_err(errors::ImplTraitPath { span: ty.span });
}

if let Some(outer_impl_trait_sp) = self.outer_impl_trait {
self.dcx().emit_err(errors::NestedImplTrait {
span: ty.span,
Expand Down Expand Up @@ -1729,7 +1684,6 @@ pub fn check_crate(
has_proc_macro_decls: false,
outer_impl_trait: None,
disallow_tilde_const: Some(TildeConstReason::Item),
is_impl_trait_banned: false,
extern_mod_safety: None,
lint_buffer: lints,
};
Expand Down
7 changes: 0 additions & 7 deletions compiler/rustc_ast_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,13 +418,6 @@ pub(crate) struct TraitObjectBound {
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(ast_passes_impl_trait_path, code = E0667)]
pub(crate) struct ImplTraitPath {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(ast_passes_nested_impl_trait, code = E0666)]
pub(crate) struct NestedImplTrait {
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_error_codes/src/error_codes/E0667.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#### Note: this error code is no longer emitted by the compiler.

`impl Trait` is not allowed in path parameters.

Erroneous code example:

```compile_fail,E0667
```ignore (removed error code)
fn some_fn(mut x: impl Iterator) -> <impl Iterator>::Item { // error!
x.next().unwrap()
}
Expand All @@ -11,7 +13,7 @@ fn some_fn(mut x: impl Iterator) -> <impl Iterator>::Item { // error!
You cannot use `impl Trait` in path parameters. If you want something
equivalent, you can do this instead:

```
```ignore (removed error code)
fn some_fn<T: Iterator>(mut x: T) -> T::Item { // ok!
x.next().unwrap()
}
Expand Down
9 changes: 0 additions & 9 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1203,15 +1203,6 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
err.emit()
} else if let Err(reported) = qself_ty.error_reported() {
reported
} else if let ty::Alias(ty::Opaque, alias_ty) = qself_ty.kind() {
// `<impl Trait as OtherTrait>::Assoc` makes no sense.
struct_span_code_err!(
self.dcx(),
tcx.def_span(alias_ty.def_id),
E0667,
"`impl Trait` is not allowed in path parameters"
)
.emit() // Already reported in an earlier stage.
Comment on lines -1207 to -1214
Copy link
Member Author

@fmease fmease Oct 27, 2024

Choose a reason for hiding this comment

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

This should've been a span_delayed_bug in the first place (before this PR). This double emission (once during AST validation, once here during HIR ty lowering) actually lead to two identical diagnostics getting emitted (indeed, even without -Zdeduplicate-diagnostics=no).

} else {
self.maybe_report_similar_assoc_fn(span, qself_ty, qself)?;

Expand Down
20 changes: 0 additions & 20 deletions tests/crashes/126725.rs

This file was deleted.

11 changes: 4 additions & 7 deletions tests/ui/impl-trait/impl_trait_projections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,27 @@ fn path_parametrized_type_is_allowed() -> option::Option<impl Debug> {
}

fn projection_is_disallowed(x: impl Iterator) -> <impl Iterator>::Item {
//~^ ERROR `impl Trait` is not allowed in path parameters
//~| ERROR `impl Trait` is not allowed in path parameters
//~^ ERROR `impl Trait` is not allowed in paths
x.next().unwrap()
}

fn projection_with_named_trait_is_disallowed(mut x: impl Iterator)
-> <impl Iterator as Iterator>::Item
//~^ ERROR `impl Trait` is not allowed in path parameters
//~^ ERROR `impl Trait` is not allowed in paths
{
x.next().unwrap()
}

fn projection_with_named_trait_inside_path_is_disallowed()
-> <::std::ops::Range<impl Debug> as Iterator>::Item
//~^ ERROR `impl Trait` is not allowed in path parameters
//~| ERROR `impl Debug: Step` is not satisfied
//~^ ERROR `impl Trait` is not allowed in paths
{
//~^ ERROR `impl Debug: Step` is not satisfied
(1i32..100).next().unwrap()
}

fn projection_from_impl_trait_inside_dyn_trait_is_disallowed()
-> <dyn Iterator<Item = impl Debug> as Iterator>::Item
//~^ ERROR `impl Trait` is not allowed in path parameters
//~^ ERROR `impl Trait` is not allowed in paths
{
panic!()
}
Expand Down
70 changes: 16 additions & 54 deletions tests/ui/impl-trait/impl_trait_projections.stderr
Original file line number Diff line number Diff line change
@@ -1,73 +1,35 @@
error[E0667]: `impl Trait` is not allowed in path parameters
error[E0562]: `impl Trait` is not allowed in paths
--> $DIR/impl_trait_projections.rs:12:51
|
LL | fn projection_is_disallowed(x: impl Iterator) -> <impl Iterator>::Item {
| ^^^^^^^^^^^^^
|
= note: `impl Trait` is only allowed in arguments and return types of functions and methods

error[E0667]: `impl Trait` is not allowed in path parameters
--> $DIR/impl_trait_projections.rs:19:9
error[E0562]: `impl Trait` is not allowed in paths
--> $DIR/impl_trait_projections.rs:18:9
|
LL | -> <impl Iterator as Iterator>::Item
| ^^^^^^^^^^^^^
|
= note: `impl Trait` is only allowed in arguments and return types of functions and methods

error[E0667]: `impl Trait` is not allowed in path parameters
--> $DIR/impl_trait_projections.rs:26:27
error[E0562]: `impl Trait` is not allowed in paths
--> $DIR/impl_trait_projections.rs:25:27
|
LL | -> <::std::ops::Range<impl Debug> as Iterator>::Item
| ^^^^^^^^^^
|
= note: `impl Trait` is only allowed in arguments and return types of functions and methods

error[E0667]: `impl Trait` is not allowed in path parameters
--> $DIR/impl_trait_projections.rs:35:29
error[E0562]: `impl Trait` is not allowed in paths
--> $DIR/impl_trait_projections.rs:32:29
|
LL | -> <dyn Iterator<Item = impl Debug> as Iterator>::Item
| ^^^^^^^^^^

error[E0667]: `impl Trait` is not allowed in path parameters
--> $DIR/impl_trait_projections.rs:12:51
|
LL | fn projection_is_disallowed(x: impl Iterator) -> <impl Iterator>::Item {
| ^^^^^^^^^^^^^

error[E0277]: the trait bound `impl Debug: Step` is not satisfied
--> $DIR/impl_trait_projections.rs:26:8
Copy link
Member Author

@fmease fmease Oct 27, 2024

Choose a reason for hiding this comment

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

Both "impl Debug: Step diagnostics" were just useless. They obscured the more important issues (impl-Trait in a forbidden context) and plastered the terminal.

|
LL | -> <::std::ops::Range<impl Debug> as Iterator>::Item
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Step` is not implemented for `impl Debug`, which is required by `std::ops::Range<impl Debug>: Iterator`
|
= help: the following other types implement trait `Step`:
Char
Ipv4Addr
Ipv6Addr
char
i128
i16
i32
i64
and 8 others
= note: required for `std::ops::Range<impl Debug>` to implement `Iterator`

error[E0277]: the trait bound `impl Debug: Step` is not satisfied
--> $DIR/impl_trait_projections.rs:29:1
|
LL | / {
LL | |
LL | | (1i32..100).next().unwrap()
LL | | }
| |_^ the trait `Step` is not implemented for `impl Debug`, which is required by `std::ops::Range<impl Debug>: Iterator`
|
= help: the following other types implement trait `Step`:
Char
Ipv4Addr
Ipv6Addr
char
i128
i16
i32
i64
and 8 others
= note: required for `std::ops::Range<impl Debug>` to implement `Iterator`
= note: `impl Trait` is only allowed in arguments and return types of functions and methods

error: aborting due to 7 previous errors
error: aborting due to 4 previous errors

Some errors have detailed explanations: E0277, E0667.
For more information about an error, try `rustc --explain E0277`.
For more information about this error, try `rustc --explain E0562`.
22 changes: 22 additions & 0 deletions tests/ui/impl-trait/in-trait/bad-projection-from-opaque.rs
Copy link
Member Author

@fmease fmease Oct 27, 2024

Choose a reason for hiding this comment

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

cc @compiler-errors since you assigned yourself to #126725. However, my PR just masks the bug by lowering the "bad" opaque to {type error}. I should come up with a different reproducer as the underlying bug still hasn't been fixed.

Copy link
Member Author

@fmease fmease Oct 27, 2024

Choose a reason for hiding this comment

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

Right, I was able to find a new reproducer for the underlying bug: #126725 (comment). I guess I will reopen #126725 once it gets closed. And I should maybe add a new crash test in this PR.

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// issue: rust-lang/rust#126725

trait Foo {
fn foo<'a>() -> <&'a impl Sized as Bar>::Output;
//~^ ERROR `impl Trait` is not allowed in paths
}

trait Bar {
type Output;
}

impl<'a> Bar for &'a () {
type Output = &'a i32;
}

impl Foo for () {
fn foo<'a>() -> <&'a Self as Bar>::Output {
&0
}
}

fn main() {}
11 changes: 11 additions & 0 deletions tests/ui/impl-trait/in-trait/bad-projection-from-opaque.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0562]: `impl Trait` is not allowed in paths
--> $DIR/bad-projection-from-opaque.rs:4:26
|
LL | fn foo<'a>() -> <&'a impl Sized as Bar>::Output;
| ^^^^^^^^^^
|
= note: `impl Trait` is only allowed in arguments and return types of functions and methods

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0562`.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
pub trait Bar { }
pub trait Quux<T> { type Assoc; }
pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
//~^ ERROR `impl Trait` is not allowed in path parameters
//~^ ERROR `impl Trait` is not allowed in paths
impl<T> Quux<T> for () { type Assoc = u32; }

fn main() { }
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
error[E0667]: `impl Trait` is not allowed in path parameters
error[E0562]: `impl Trait` is not allowed in paths
--> $DIR/issue-57979-impl-trait-in-path.rs:8:48
|
LL | pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
| ^^^^^^^^
|
= note: `impl Trait` is only allowed in arguments and return types of functions and methods

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0667`.
For more information about this error, try `rustc --explain E0562`.
Loading