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

Type and const parameters are not used as a fallback during type inference #98931

Open
WalterSmuts opened this issue Jul 5, 2022 · 15 comments
Open
Labels
A-inference Area: Type inference C-feature-request Category: A feature request, i.e: not implemented / a PR. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@WalterSmuts
Copy link

Currently you're able to define default const parameters for a type. This is explained here. This allows one to write generic impl blocks but requires some explicit type coercion as shown in this stack-overflow article on the new constructor.

It would be great if rust can infer that the type has to be the return type of the new constructor and, if the const parameter is not provided, infer that the type defaults to the default const value.

The stack-overflow article had me confused for a bit (see this reddit post) so I'll provide a more explicit example:

#[derive(Debug)]
struct Foo<const N: usize = 1> {
    set_on_creation: bool,
    passed_in: bool,
    generic_field: [usize; N],
}

impl<const N: usize> Foo<N> {
    pub fn new(passed_in: bool) -> Self {
        Self {
            set_on_creation: true,
            passed_in,
            generic_field: [0; N],
        }
    }
}

fn main() {
    let a = Foo::new(true);
    dbg!(a);
    let b = Foo::new(false);
    dbg!(b);
    let c: Foo<2> = Foo::new(false);
    dbg!(c);
}

I expected to see this happen:
Successfully infer that the type has to be the default const variant of the type.

Instead, this happened:

error[E0282]: type annotations needed for `Foo<N>`
  --> src/main.rs:19:13
   |
19 |     let a = Foo::new(true);
   |         -   ^^^^^^^^ cannot infer the value of const parameter `N`
   |         |
   |         consider giving `a` the explicit type `Foo<N>`, where the const parameter `N` is specified

Clarity

To be clear, the following works:

    let a: Foo = Foo::new(true);

but this does not:

    let a = Foo::new(true);

On the face of it the above looks silly.

Meta

Bug is also present on nightly.

[walter@cuddles const-generic-default-playground]$ rustc --version --verbose
rustc 1.60.0 (7737e0b5c 2022-04-04)
binary: rustc
commit-hash: 7737e0b5c4103216d6fd8cf941b7ab9bdbaace7c
commit-date: 2022-04-04
host: x86_64-unknown-linux-gnu
release: 1.60.0
LLVM version: 14.0.0
@WalterSmuts WalterSmuts added the C-bug Category: This is a bug. label Jul 5, 2022
@WalterSmuts WalterSmuts changed the title Type inference on cont generic defaults Type inference on const generic defaults Jul 5, 2022
@fmease
Copy link
Member

fmease commented Jul 5, 2022

Duplicate of #83687, #96300.

WalterSmuts added a commit to WalterSmuts/pitch-corrector that referenced this issue Jul 7, 2022
Use const generics to make the display processor buffer defined by user.
Unfortunately the default const type inference makes working with this
clunky. See issue: rust-lang/rust#98931
@JulianKnodt
Copy link
Contributor

@fmease can we move these to one issue, and I can try to fix it? I didn't realize this didn't work

@fmease
Copy link
Member

fmease commented Jul 9, 2022

@JulianKnodt The thing is – if I remember correctly – that this issue / feature isn't a trivial one to fix / implement and that it first requires some design work / large architectural changes to rustc's inference engine. This topic has already been discussed somewhere sometime ago (sadly I can't link to anything). There might also be concerns around backward compatibility and inference ambiguities.

I would suggest you to search and ask on Zulip or on the internals forum for some context, background information or initial guidance maybe.

@fmease
Copy link
Member

fmease commented Jul 9, 2022

It would probably also require a major change proposal (MCP) or an RFC.

@JulianKnodt
Copy link
Contributor

Chalk currently has no way to handle generic consts, and I was working on adding that (altho I got sidetracked and haven't actually put in work to get it closer to working), but it would still be nice to track these issues under const-generics or const_generics_defaults

@WalterSmuts
Copy link
Author

@JulianKnodt Please can you fix this! IMO this is such a sore finger in rust language.

FYI, this probably needs to go hand-in hand with default generic type inference too. For example see the following code on normal generic (non-const generic) types:

#[derive(Default)]
struct MyTypeWrapper<T = i32>(T);

impl<T> MyTypeWrapper<T> {
    const fn new(t: T) -> Self {
        Self(t)
    }
}

fn main() {
    // This does work but is needlessly verbose IMO
    let a: MyTypeWrapper = MyTypeWrapper::default();

    // This does not work but should IMO
    let a = MyTypeWrapper::default();

    // This should work and does work
    let a: MyTypeWrapper<usize> = MyTypeWrapper::default();
}

The fact that rust behaves like this for normal generic default types is why #83687 was closed.

@WalterSmuts WalterSmuts changed the title Type inference on const generic defaults Type inference on [const] generic defaults Jul 10, 2022
@JulianKnodt
Copy link
Contributor

JulianKnodt commented Jul 10, 2022

Hmmmm, I didn't realize that generic default types also behaved that way, but it might be worth it to just add a note to the error message.

Edit: I'm wondering whether it would be possible to just fix this by first trying to infer the generic args without defaults, and if it fails substitute defaults one by one and see if any succeeds?

WalterSmuts added a commit to WalterSmuts/pitch-corrector that referenced this issue Jul 23, 2022
Still requires nightly:
rust-lang/rust#76560

Type inference is lacking on default values:
rust-lang/rust#98931
@TimJentzsch
Copy link

We ran into an issue that would benefit from const generic defaults working on generic functions as well.

We had a struct with a field of petitset::PetitSet which itself has a const generic maximum size. In our code, this was hard-coded to 8.
We now wanted to make this generic instead, to give the user more control of the maximum size.
To avoid ergonomic regressions and breaking changes, we planned to provide a default value of 8 to the generic, so that it would be a seamless transitions for the users.

However, because the default generic value cannot be derived for the constructor methods, the users would have to add a new type annotation or specify the size explicitly to avoid errors.
Thus, it would be a breaking change and be more verbose to write, so we have to hold back this feature for now.

Related PR is at Leafwing-Studios/leafwing-input-manager#241

@JulianKnodt
Copy link
Contributor

JulianKnodt commented Sep 4, 2022

@TimJentzsch
After some discussion about it, it would make sense to add a warning that if the parameter is missing but has a default, it still needs to be explicitly specified. Instead, what you may be able to do is have two constructors, one specifically implemented for your default:

impl YourStruct<8 /* Or whatever your default is */> {
  fn new() -> Self {
    ...
  }
}

And one generic one:

impl<const N: usize> YourStruct<N> {
  fn new_non_default() -> Self {
     ...
  }
}

While it is less elegant to have 2 separate constructors with different names, it should be back-compatible and will allow you to differentiate and have changes for different sizes between the new constructor and the old constructor without breaking older users.

Playground Reference

@alice-i-cecile
Copy link

That's an okay (if inelegant) workaround for now. Is it impossible / impractical to make this default value inferred?

@JulianKnodt
Copy link
Contributor

I think currently there is no movement to do so, because it would lead to a mismatch between inferred consts and inferred type, neither of which is inferred on an impl. I also personally feel that in this case inference to the default makes sense and is intuitive, but I can't make the change on my own, it would require an MCP as @fmease says

@MoSal
Copy link

MoSal commented Mar 22, 2023

This limitation can be weird sometimes.

struct Foo<const A: usize, const B: usize = 0>;

impl<const B: usize> Foo<0, B> {
    fn mk() -> Self { Self }
}

fn main() {
    // complains about unspecified B
    let f = Foo::mk();

    // works, and that zero is not B's
    let f: Foo<0> = Foo::mk();
}

@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-inference Area: Type inference and removed needs-triage-legacy labels Sep 6, 2023
@fmease fmease changed the title Type inference on [const] generic defaults Type and const parameters are not used as a fallback during type inference Jan 23, 2024
@fmease fmease added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-types Relevant to the types team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed C-bug Category: This is a bug. labels Jan 23, 2024
@fmease fmease added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 23, 2024
@fmease
Copy link
Member

fmease commented Jan 23, 2024

I'm now using this as a hub for duplicate bug reports and for discussions around #![feature(default_type_parameter_fallback)] ( F-default_type_parameter_fallback `#![feature(default_type_parameter_fallback)]` ) superseding the closed tracking issue #27336.

github-merge-queue bot pushed a commit to CQCL/hugr that referenced this issue Jul 17, 2024
…e(/Row)/etc. (#1138)

* Type now means a single type, not a row variable; TypeRV can be a
single type *or* a row variable. (Type --Into--> TypeRV).
* These are actually parameterizations: `TypeBase<NoRV>` (NoRV is a type
with no instances) vs `TypeBase<RowVariable>` (RowVariable is a struct
i.e. index + bound)
* Similarly for the other type structures - all names have changed
uniformly except FunctionType:

| No Row Vars | With Row Vars | Generic |
| ----- | ----- | ----- |
| Type | TypeRV | `TypeBase<RV>` |
| TypeRow | TypeRowRV | `TypeRowBase<RV>` |
| Signature | FuncValueType | `FuncTypeBase<RV>` |
| PolyFuncType | PolyFuncTypeRV | `PolyFuncTypeBase<RV>` |

* This allows us to make static guarantees (and remove dynamic checks):
    * node signatures do not have row variables as inputs or outputs
* Neither do FuncDefn type schemes, even though they may quantify over
row variables
* "port"-indexing methods are specific to Signature (not FuncValueType)
- this justifies the exceptional naming

Planning to rename PolyFuncType to (Op/Fn)TypeScheme in a followup PR
(lots of fields want renaming too, whereas many FunctionType things are
*already* called `signature`!)

**One question** is that currently `Type` and `TypeRV` are aliases for
instantiations of `TypeBase`. An alternative is to parameterize
`Type<RV: MaybeRV = NoRV>` and define as alias only `type TypeRV =
Type<RowVariable>` (no `TypeBase`). A type annotation e.g. `x: Type`
then uses the default, but a call like `Type::new_function` then tries
to infer a unique possible parameter. This would slightly worsen
Hugr-building code (some code like `let x = Type::new_function` have to
become either `let x = Type::<NoRV>::new_function` or `let x: Type =
Type::new_function` - sadly the *default* for RV is ignored for
members/namespacing but not when forming a type, see
rust-lang/rust#98931). OTOH some OpDef code
would be a bit easier (needs fewer explicit `RV` suffices). In general I
think we favour making Hugr-building code as easy as possible over OpDef
code, but there is always the question of, *how much* easier etc...see
commit bc565ba which removed defaults
for Type (with an earlier parametrization by bool true/false rather than
by RowVariable/NoRV). (And actually that commit understates the
simplification - there were a bunch more `let x:Type`s required for the
default method that can be removed for the always-explicit method, I
took these out in 23db41d, so maybe we
should stick with always-explicit.)

* [ ] TODO python classes need names updating to match, etc. 

BREAKING CHANGE: (1) In OpDefs, Type/TypeRow/FunctionType/PolyFuncType
will generally need replacing by
TypeRV/TypeRowRV/FuncValueType/PolyFuncTypeRV. (2) FuncValueType omits
the various numbered-"port" accessors, you really shouldn't be doing
that except on a Signature.... (4)
SignatureError::RowVarWhereTypeExpected now contains a RowVariable
struct (including bound) rather than only the index
@goffrie
Copy link
Contributor

goffrie commented Oct 1, 2024

Oddly, it does work if you use the syntax <Foo>::new(), or define a type alias type Bar = Foo; Bar::new().

@fmease
Copy link
Member

fmease commented Oct 1, 2024

Right, that's because the omission of path arguments is handled differently depending on the context, namely on whether they're in "type position" or in "expression position".

In type position, no type inference occurs when "expanding" the generic arguments and one has to supply arguments for all non-lifetime non-defaulted parameters and parameter defaults are considered (for lifetime parameters, elision is permitted (well, in even fewer contexts that is)). That's your <Ty>::assoc and your type Al = Ty.

In expression position, a lack of generic arguments leads to the compiler supplying inference variables for all generic parameters (I'm ignoring lifetime elision here). These inference variables don't know a lot about their origin starting out, most notably the fact that there could be "defaults" for them. They are disconnected from that notion, so naturally there's no way for the type checker to consider the defaults.

And while we could almost trivially equip their origin (e.g. TypeVariableOrigin in the compiler) with a notion of defaults, the problem lies in the question of when exactly to apply said default during the inference process. This is why there has been no progress whatsoever on this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inference Area: Type inference C-feature-request Category: A feature request, i.e: not implemented / a PR. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants