fix(mgca): Allow specifying generic args (of enum) on enum itself in unit & tuple variant constructions in (direct) const args

[JayanAXHF]

View all comments

Closes #154915

Basically, we check if there are three segments and if the first one is a DefKind::Enum, and then use the correct segment for Enum::<...>::Variant syntax for both DefKind::Ctor(_, CtorKind::Const | CtorKind::Fn)

PS: i'm not sure if the common logic from these two branches could be extracted

r? fmease

[rustbot]

HIR ty lowering was modified

cc @fmease

[JayanAXHF]

Fixed that @fmease

[fmease]

Thanks for working on this! I got some thoughts, apologies for the many comments; most of them are really minor that can be fixed super quickly.

View changes since this review

[fmease]

The path should be allowed to have more than two segments. Right now, you're only allowing Enum::<...>::Variant but not e.g., very::long::path::to::Enum::<...>::Variant. So it should be >= 2 rather than == 2.

Once you implement that you also need to make sure to reject arguments on all segments before the enum segment since module::<...>::Enum::<...>::Variant should be forbidden.

[JayanAXHF]

@rustbot ready

[fmease]

I'm so sorry for the delay! Some final comments

View changes since this review

[fmease]

I'm glad my suggestion to use probe_generic_path_segments is working out.

However, calling that function isn't sufficient, you still need (manually) prohibit generic args on all path segments that aren't generic using prohibit_generic_args, I know this API is a bit awkward. See the preexisting callers of probe_generic_path_segments for how they do it.

(I guess it could be nice if we'd have one function that does the "probing" and the "prohibiting" at once for convenience but I don't think we can do that yet w/o refactoring some things first, so that has to wait for another time)

[JayanAXHF]

I don't think we can do that yet w/o refactoring some things first

I believe we can. Currently, how this is done is

                let generic_segments =
                    self.probe_generic_path_segments(path.segments, opt_self_ty, kind, did, span);
                let indices: FxHashSet<_> =
                    generic_segments.iter().map(|GenericPathSegment(_, index)| index).collect();
                let _ = self.prohibit_generic_args(
                    path.segments.iter().enumerate().filter_map(|(index, seg)| {
                        if !indices.contains(&index) { Some(seg) } else { None }
                    }),
                    GenericsArgsErrExtend::DefVariant(&path.segments),
                );

I believe this could just be turned into a function

[fmease]

Ah, in that case, go for it! Unless you'd to leave it to a follow-up PR; this way we can merge this PR faster :)

[JayanAXHF]

Let's go for a follow-up PR. I'll create an issue so i don't forget if that's okay

[JayanAXHF]

apologies for the delay, will look into this on the weekend

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[JayanAXHF]

Made the changes @fmease

@rustbot ready

[fmease]

Apologies for the delay, two final nits and one other note.

View changes since this review

[fmease]

Ah, in that case, go for it! Unless you'd to leave it to a follow-up PR; this way we can merge this PR faster :)

[fmease]

Thanks for your work and your patience! :)

I've --bless'ed the failing test since that was all it needed. and I took the liberty to squash your commits. @bors r+ rollup

[rust-bors]

📌 Commit 7fe4408 has been approved by fmease

It is now in the queue for this repository.

[JonathanBrouwer]

@rust-timer build 4858221

[rust-timer]

Finished benchmarking commit (4858221): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -0.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-3.0%, 2.9%]	2

Cycles

Results (secondary -2.8%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 517.006s -> 518.123s (0.22%)
Artifact size: 401.43 MiB -> 400.92 MiB (-0.13%)