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

Accept Enum::<Param>::Variant {} over Enum::Variant::<Param> {} #69363

Closed
wants to merge 4 commits into from
Closed
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
4 changes: 3 additions & 1 deletion src/librustc_ast_lowering/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1977,6 +1977,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
args: &[],
bindings: arena_vec![self; self.output_ty_binding(span, output_ty)],
parenthesized: false,
span,
});

// ::std::future::Future<future_params>
Expand Down Expand Up @@ -2655,11 +2656,12 @@ impl<'hir> GenericArgsCtor<'hir> {
self.args.is_empty() && self.bindings.is_empty() && !self.parenthesized
}

fn into_generic_args(self, arena: &'hir Arena<'hir>) -> hir::GenericArgs<'hir> {
fn into_generic_args(self, arena: &'hir Arena<'hir>, span: Span) -> hir::GenericArgs<'hir> {
hir::GenericArgs {
args: arena.alloc_from_iter(self.args),
bindings: self.bindings,
parenthesized: self.parenthesized,
span,
}
}
}
21 changes: 19 additions & 2 deletions src/librustc_ast_lowering/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
self.resolver.get_partial_res(id).unwrap_or_else(|| PartialRes::new(Res::Err));

let proj_start = p.segments.len() - partial_res.unresolved_segments();
let mut type_param_in_enum = false;
let path = self.arena.alloc(hir::Path {
res: self.lower_res(partial_res.base_res()),
segments: self.arena.alloc_from_iter(p.segments[..proj_start].iter().enumerate().map(
Expand All @@ -55,7 +56,21 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
Res::Def(DefKind::AssocTy, def_id) if i + 2 == proj_start => {
Some(parent_def_id(self, def_id))
}
Res::Def(DefKind::Variant, def_id) if i + 1 == proj_start => {
Res::Def(DefKind::Variant, def_id)
if i + 2 == proj_start && segment.args.is_some() =>
{
// Handle `Enum::<Param>::Variant {}`.
// We need to choose one here so that the method `res_to_ty` in
// `astconv` works correctly, but it has to be *only* one, so track
// whether we've already set the `DefId` in the previous segment.
type_param_in_enum = true;
Some(parent_def_id(self, def_id))
}
Res::Def(DefKind::Variant, def_id)
if i + 1 == proj_start
&& (segment.args.is_some() || !type_param_in_enum) =>
{
// Handle `Enum::Variant::<Param> {}`.
Some(parent_def_id(self, def_id))
}
estebank marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +59 to 75
Copy link
Member

@eddyb eddyb Feb 25, 2020

Choose a reason for hiding this comment

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

Two things here:

  1. you can avoid mutable state by looking at p.segments
    • btw I doubt a bit that .args.is_some() is the correct way to check this, it might be true even for ::<> which I'm not sure if it's compatible with the similar check in typeck
  2. these only make sense if the param mode is explicit, which it can never be for Expr::Struct right now (<Enum>::Variant {...} is not valid syntax, and Enum::Variant is not valid in type paths yet, only one of these two could actually be relevant here)
    • the comments referring to Enum::Variant {...} are therefore inaccurate IMO
    • this needs some extra checks, probably for the param mode being explicit
    • it's really mostly forward compat for enum variant types so you could just as well add an assert that DefKind::Variant parts are never ParamMode::Explicit

Res::Def(DefKind::Struct, def_id)
Expand Down Expand Up @@ -345,6 +360,8 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
segment.ident, segment.id, id,
);

let param_sp =
segment.args.as_ref().map_or(segment.ident.span.shrink_to_hi(), |args| args.span());
hir::PathSegment {
ident: segment.ident,
hir_id: Some(id),
Expand All @@ -353,7 +370,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
args: if generic_args.is_empty() {
None
} else {
Some(self.arena.alloc(generic_args.into_generic_args(self.arena)))
Some(self.arena.alloc(generic_args.into_generic_args(self.arena, param_sp)))
},
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_error_codes/error_codes/E0109.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ type X = u32; // ok!
type Y = bool; // ok!
```

Note that generic arguments for enum variant constructors go after the variant,
not after the enum. For example, you would write `Option::None::<u32>`,
rather than `Option::<u32>::None`.
Note that generic arguments for enum variant constructors can go after the
variant or after the enum. For example, you can write `Option::None::<u32>`,
rather than `Option::<u32>::None`, but the later style is preferred.
4 changes: 3 additions & 1 deletion src/librustc_hir/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,11 +311,13 @@ pub struct GenericArgs<'hir> {
/// This is required mostly for pretty-printing and diagnostics,
/// but also for changing lifetime elision rules to be "function-like".
pub parenthesized: bool,
/// The `Span` encompassing the entirety of the parameters `<A, B>` or `(A, B)`.
pub span: Span,
}

impl GenericArgs<'_> {
pub const fn none() -> Self {
Self { args: &[], bindings: &[], parenthesized: false }
Self { args: &[], bindings: &[], parenthesized: false, span: DUMMY_SP }
}

pub fn is_empty(&self) -> bool {
Expand Down
8 changes: 8 additions & 0 deletions src/librustc_session/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,13 @@ declare_lint! {
};
}

declare_lint! {
pub TYPE_PARAM_ON_VARIANT_CTOR,
Allow,
"detects generic arguments in path segments corresponding to an enum variant instead of the \
enum itself",
}

declare_lint! {
pub ORDER_DEPENDENT_TRAIT_OBJECTS,
Deny,
Expand Down Expand Up @@ -531,6 +538,7 @@ declare_lint_pass! {
PATTERNS_IN_FNS_WITHOUT_BODY,
MISSING_FRAGMENT_SPECIFIER,
LATE_BOUND_LIFETIME_ARGUMENTS,
TYPE_PARAM_ON_VARIANT_CTOR,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I would include CTOR in the name. Also, this isn't limited to type parameters.

I'd call it VARIANT_GENERICS or VARIANT_GENERIC_ARGS.

ORDER_DEPENDENT_TRAIT_OBJECTS,
COHERENCE_LEAK_CHECK,
DEPRECATED,
Expand Down
108 changes: 89 additions & 19 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ enum ConvertedBindingKind<'a, 'tcx> {
Constraint(&'a [hir::GenericBound<'a>]),
}

#[derive(PartialEq)]
enum GenericArgPosition {
#[derive(PartialEq, Debug)]
pub enum GenericArgPosition {
Type,
Value, // e.g., functions
MethodCall,
Expand Down Expand Up @@ -195,6 +195,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
span: Span,
def_id: DefId,
item_segment: &hir::PathSegment<'_>,
generic_arg_position: GenericArgPosition,
) -> SubstsRef<'tcx> {
let (substs, assoc_bindings, _) = self.create_substs_for_ast_path(
span,
Expand All @@ -203,6 +204,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
item_segment.generic_args(),
item_segment.infer_args,
None,
generic_arg_position,
);

assoc_bindings.first().map(|b| Self::prohibit_assoc_ty_binding(self.tcx(), b.span));
Expand Down Expand Up @@ -630,6 +632,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
generic_args: &'a hir::GenericArgs<'_>,
infer_args: bool,
self_ty: Option<Ty<'tcx>>,
generic_arg_position: GenericArgPosition,
) -> (SubstsRef<'tcx>, Vec<ConvertedBinding<'a, 'tcx>>, Option<Vec<Span>>) {
// If the type is parameterized by this region, then replace this
// region with the current anon region binding (in other words,
Expand Down Expand Up @@ -661,7 +664,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
span,
&generic_params,
&generic_args,
GenericArgPosition::Type,
generic_arg_position,
self_ty.is_some(),
infer_args,
);
Expand Down Expand Up @@ -804,6 +807,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
item_def_id: DefId,
item_segment: &hir::PathSegment<'_>,
parent_substs: SubstsRef<'tcx>,
generic_arg_position: GenericArgPosition,
) -> SubstsRef<'tcx> {
if tcx.generics_of(item_def_id).params.is_empty() {
self.prohibit_generics(slice::from_ref(item_segment));
Expand All @@ -817,6 +821,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
item_segment.generic_args(),
item_segment.infer_args,
None,
generic_arg_position,
)
.0
}
Expand Down Expand Up @@ -1100,6 +1105,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
trait_segment.generic_args(),
trait_segment.infer_args,
Some(self_ty),
GenericArgPosition::Type,
)
}

Expand Down Expand Up @@ -1416,8 +1422,9 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
span: Span,
did: DefId,
item_segment: &hir::PathSegment<'_>,
generic_arg_position: GenericArgPosition,
) -> Ty<'tcx> {
let substs = self.ast_path_substs_for_ty(span, did, item_segment);
let substs = self.ast_path_substs_for_ty(span, did, item_segment, generic_arg_position);
self.normalize_ty(span, self.tcx().at(span).type_of(did).subst(self.tcx(), substs))
}

Expand Down Expand Up @@ -2253,6 +2260,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
item_def_id: DefId,
trait_segment: &hir::PathSegment<'_>,
item_segment: &hir::PathSegment<'_>,
generic_arg_position: GenericArgPosition,
) -> Ty<'tcx> {
let tcx = self.tcx();

Expand Down Expand Up @@ -2305,13 +2313,36 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
item_def_id,
item_segment,
trait_ref.substs,
generic_arg_position,
);

debug!("qpath_to_ty: trait_ref={:?}", trait_ref);

self.normalize_ty(span, tcx.mk_projection(item_def_id, item_substs))
}

pub fn prohibit_multiple_params<'a, T: IntoIterator<Item = &'a hir::PathSegment<'a>>>(
&self,
segments: T,
) -> bool {
let segments_with_params = segments
.into_iter()
.filter_map(|segment| segment.args.map(|arg| arg.span))
.collect::<Vec<_>>();
if segments_with_params.len() <= 1 {
return false;
}
self.tcx()
.sess
.struct_span_err(
segments_with_params,
"multiple segments with type parameters are not allowed",
)
.note("only a single path segment can specify type parameters")
.emit();
true
}
Comment on lines +2324 to +2344
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unnecessarily complicated, it doesn't distinguish this "enum vs variant" situation from others (e.g. trying to pass generic args to a mod) and it's wrong with generic associated items like methods or GATs.

This could be a help message on the error from prohibit_generics, such as pointing to the enum segment and saying "this segment already specified generic args for the enum" or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is unnecessarily complicated, it doesn't distinguish this "enum vs variant" situation from others (e.g. trying to pass generic args to a mod) and it's wrong with generic associated items like methods or GATs.

That check is being done when called. If it were called without that check it would indeed be incorrect.

This could be a help message on the error from prohibit_generics, such as pointing to the enum segment and saying "this segment already specified generic args for the enum" or something.

Wanted to separate this from E0109 to potentially give it its own error code to explain that this will never be supported as opposed to the more generic current error.

Copy link
Member

Choose a reason for hiding this comment

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

to explain that this will never be supported

What do you mean? Generics on both the enum and a variant is a plausible feature (GADT), and generics on modules have been talked about in the past.

The error message looks pretty misleading to me, it seems to claim some absolute truth.


pub fn prohibit_generics<'a, T: IntoIterator<Item = &'a hir::PathSegment<'a>>>(
&self,
segments: T,
Expand Down Expand Up @@ -2469,8 +2500,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
let enum_def_id = tcx.parent(def_id).unwrap();
(enum_def_id, last - 1)
} else {
// FIXME: lint here recommending `Enum::<...>::Variant` form
// instead of `Enum::Variant::<...>` form.
self.lint_type_param_on_variant_ctor(segments);

// Everything but the final segment should have no
// parameters at all.
Expand Down Expand Up @@ -2504,18 +2534,51 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
path_segs
}

fn lint_type_param_on_variant_ctor(&self, segments: &[hir::PathSegment<'_>]) {
// Doing this to get around rustfmt-caused line too long.
use hir::PathSegment as P;
if let [.., prev, P { ident, hir_id: Some(hir_id), args: Some(args), .. }] = segments {
let sp = args.span;
if sp.hi() == sp.lo() || sp == DUMMY_SP || sp.parent().is_some() {
// The params were not written by the user, but rather derived. These are expected.
// `Enum::Variant` where `Variant` has inferred lifetimes.
return;
}
Comment on lines +2542 to +2546
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this check, and you can get rid of the lifetimes for now (they shouldn't be added to value paths anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. It's hacky as hell.

you can get rid of the lifetimes for now

How?

they shouldn't be added to value paths anyway

I've spent too much time trying to figure out of a way of making this change without affecting currently accepted code.

Copy link
Member

Choose a reason for hiding this comment

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

Basically you want all Expr::Struct with variant paths to behave in HRI lowering like they did when you had removed the DefKind::Variant arm.
You don't want the elision lifetimes being injected at all.

They're for fn foo(x: &T) -> Enum::Variant, or perhaps <Enum>::Variant {...} (neither of which compile right now), not Enum::Variant {...}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if none of the PathSegments have the elision lifetimes injected (in src/librustc_ast_lowering/path.rs) then you get knock down errors about the type for a HirId not existing .

Copy link
Member

Choose a reason for hiding this comment

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

then you get knock down errors about the type for a HirId not existing

I don't recall you bringing this up, I thought it just worked while you had that arm removed? I don't see why it would, other value paths have no lifetimes injected into them at all.

let span = sp.with_lo(ident.span.hi()); // Account for `::`
self.tcx().struct_span_lint_hir(
lint::builtin::TYPE_PARAM_ON_VARIANT_CTOR,
*hir_id,
sp,
|lint| {
let mut err = lint.build("type parameter on variant");
let sugg_span = prev.ident.span.shrink_to_hi();
let msg = "set the type parameter on the enum";
match self.tcx().sess.source_map().span_to_snippet(span) {
Ok(snippet) => err.multipart_suggestion(
msg,
vec![(sugg_span, snippet), (span, "".to_string())],
Applicability::MachineApplicable,
),
Err(_) => err.span_label(sugg_span, msg),
};
err.emit();
},
);
}
}

// Check a type `Path` and convert it to a `Ty`.
pub fn res_to_ty(
&self,
opt_self_ty: Option<Ty<'tcx>>,
path: &hir::Path<'_>,
permit_variants: bool,
generic_arg_position: GenericArgPosition,
) -> Ty<'tcx> {
let tcx = self.tcx();

debug!(
"res_to_ty(res={:?}, opt_self_ty={:?}, path_segments={:?})",
path.res, opt_self_ty, path.segments
"res_to_ty(res={:?}, opt_self_ty={:?}, path_segments={:?} generic_arg_pos={:?})",
path.res, opt_self_ty, path.segments, generic_arg_position
);

let span = path.span;
Expand All @@ -2525,7 +2588,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
assert!(ty::is_impl_trait_defn(tcx, did).is_none());
let item_segment = path.segments.split_last().unwrap();
self.prohibit_generics(item_segment.1);
let substs = self.ast_path_substs_for_ty(span, did, item_segment.0);
let substs =
self.ast_path_substs_for_ty(span, did, item_segment.0, generic_arg_position);
self.normalize_ty(span, tcx.mk_opaque(did, substs))
}
Res::Def(DefKind::Enum, did)
Expand All @@ -2535,9 +2599,11 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
| Res::Def(DefKind::ForeignTy, did) => {
assert_eq!(opt_self_ty, None);
self.prohibit_generics(path.segments.split_last().unwrap().1);
self.ast_path_to_ty(span, did, path.segments.last().unwrap())
self.ast_path_to_ty(span, did, path.segments.last().unwrap(), generic_arg_position)
}
Res::Def(kind @ DefKind::Variant, def_id) if permit_variants => {
Res::Def(kind @ DefKind::Variant, def_id)
if generic_arg_position == GenericArgPosition::Value =>
{
// Convert "variant type" as if it were a real type.
// The resulting `Ty` is type of the variant's enum for now.
assert_eq!(opt_self_ty, None);
Expand All @@ -2546,14 +2612,17 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
self.def_ids_for_value_path_segments(&path.segments, None, kind, def_id);
let generic_segs: FxHashSet<_> =
path_segs.iter().map(|PathSeg(_, index)| index).collect();
self.prohibit_generics(path.segments.iter().enumerate().filter_map(
|(index, seg)| {
if !generic_segs.contains(&index) { Some(seg) } else { None }
},
));

if !self.prohibit_multiple_params(path.segments.iter()) {
self.prohibit_generics(path.segments.iter().enumerate().filter_map(
|(index, seg)| {
if !generic_segs.contains(&index) { Some(seg) } else { None }
},
));
}

let PathSeg(def_id, index) = path_segs.last().unwrap();
self.ast_path_to_ty(span, *def_id, &path.segments[*index])
self.ast_path_to_ty(span, *def_id, &path.segments[*index], generic_arg_position)
}
Res::Def(DefKind::TyParam, def_id) => {
assert_eq!(opt_self_ty, None);
Expand Down Expand Up @@ -2588,6 +2657,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
def_id,
&path.segments[path.segments.len() - 2],
path.segments.last().unwrap(),
generic_arg_position,
)
}
Res::PrimTy(prim_ty) => {
Expand Down Expand Up @@ -2642,7 +2712,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
hir::TyKind::Path(hir::QPath::Resolved(ref maybe_qself, ref path)) => {
debug!("ast_ty_to_ty: maybe_qself={:?} path={:?}", maybe_qself, path);
let opt_self_ty = maybe_qself.as_ref().map(|qself| self.ast_ty_to_ty(qself));
self.res_to_ty(opt_self_ty, path, false)
self.res_to_ty(opt_self_ty, path, GenericArgPosition::Type)
}
hir::TyKind::Def(item_id, ref lifetimes) => {
let did = tcx.hir().local_def_id(item_id.id);
Expand Down
Loading