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

Add better diagnostic for const-generic impl Foo for S<N> #82264

Open
JulianKnodt opened this issue Feb 18, 2021 · 5 comments
Open

Add better diagnostic for const-generic impl Foo for S<N> #82264

JulianKnodt opened this issue Feb 18, 2021 · 5 comments
Labels
A-const-generics Area: const generics (parameters and arguments) A-diagnostics Area: Messages for errors, warnings, and lints A-resolve Area: Path resolution A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@JulianKnodt
Copy link
Contributor

JulianKnodt commented Feb 18, 2021

Adding better diagnostics when we resolve to a res::Err for N.

Original comment:
Ideally this would suggest "you meant to add a const N here and write this as A<{ N }>".

Originally posted by @estebank in #82055 (comment)

@estebank
Copy link
Contributor

Given

struct A<const N: u8>;
impl Foo for A<N> {}

we should suggest

impl<const N: u8> Foo for A<{ N }> {}

@estebank estebank added A-const-generics Area: const generics (parameters and arguments) A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-resolve Area: Path resolution labels Feb 18, 2021
@slightlyoutofphase
Copy link
Contributor

slightlyoutofphase commented Feb 18, 2021

It's worth noting that the impl part is specifically the error there... the rest is fine. I don't think the "const braces" are required anywhere at all currently (except maybe for particularly complex stuff, not quite sure). For example:

trait Foo<const N1: u8, const N2: u8, const N3: u8> {}

struct A<const N1: u8, const N2: u8, const N3: u8>;

type AA = A<1, 2, 3>;

// The compiler actively tells you that the braces on the next line are unnecessary, currently.
type AAA = A<{ 1 }, { 2 }, { 3 }>;

// The braces on the next line are equally unnecessary, but not warned against the way they are
// above for some reason.
impl<const N1: u8, const N2: u8, const N3: u8> Foo<N1, N2, N3> for A<{ N1 }, { N2 }, { N3 }> {}

@JulianKnodt
Copy link
Contributor Author

they are required for anything more than plain ints such as { [0u8, 1u8] }, of { foo(N) }. For the general type I think it'd be nicer to suggest braces if we don't have any prior info.

@slightlyoutofphase
Copy link
Contributor

slightlyoutofphase commented Feb 19, 2021

I tried out a few different things later on yesterday, and it seems like the compiler does aleady suggest adding "const braces" in situations where they might actually help.

However, in the scenarios above, they wouldn't help, and as far as I can tell the compiler is fully aware of that since the only place they could be added is not a place that has anything to do with what's causing the syntax error in the first place.

Ultimately either way of displaying the new suggestion is fine though of course, as they're both equally valid code, so it doesn't really make a difference either way.

@estebank
Copy link
Contributor

You're right. Single ident paths are accepted without braces and have always been meant to be so. suggesting just the addition of the const arguments to the impl block would be enough for this case.

@JohnTitor JohnTitor added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) A-diagnostics Area: Messages for errors, warnings, and lints A-resolve Area: Path resolution A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. 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