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 annotations "look through" typedefs, which can be confusing. #1666

Open
umanwizard opened this issue Aug 8, 2019 · 23 comments
Open

Type annotations "look through" typedefs, which can be confusing. #1666

umanwizard opened this issue Aug 8, 2019 · 23 comments
Labels
A-ty type system / type inference / traits / method resolution E-unknown It's unclear if the issue is E-hard or E-easy without digging in S-unactionable Issue requires feedback, design decisions or is blocked on other work

Comments

@umanwizard
Copy link
Contributor

blah.rs:

pub struct Foo;

lib.rs:

mod blah;
type BlahFoo = blah::Foo;
pub struct Foo {
    inner: BlahFoo,
}
pub fn f(f: Foo) {
    let _x = f.inner;
}

Rust-analyzer reports the type of _x to be Foo, which is quite confusing -- it would be better as BlahFoo or even blah::Foo

@matklad matklad closed this as completed Aug 9, 2019
@matklad matklad added the E-unknown It's unclear if the issue is E-hard or E-easy without digging in label Aug 9, 2019
@matklad matklad reopened this Aug 9, 2019
@matklad
Copy link
Member

matklad commented Aug 9, 2019

Yeah, I wonder what is the correct way to deal with type aliases....

Currently we eagerly normalize them, which causes the reported issue. I wonder if we should have TypeAlias as a variant of TypeCtor::Adt? That means though, that we'd need to adjust type equality, unification to properly deal with aliases.

That should make the following code possible ideally (which is currently an error in rustc):

type List<T> = (T, Box<Option<List<T>>>);

@flodiebold
Copy link
Member

I personally don't think we should treat type aliases as a separate type. That seems like too much of a change from Rust's actual type system. (Also note that type aliases can alias to any type, not just ADTs.)

What I imagine is keeping track of where a type came from during type inference (either using a side table or using additional fields on the types), and then using that to represent the type in the way it was originally written (i.e. with type aliases, using the same path, leaving out default type parameters if they were left out there, etc.). Though I have to admit it's only a vague notion so far 😄

@flodiebold
Copy link
Member

(Or maybe put another way, it seems a layering violation to fix a 'display' issue like this by changing the type system.)

@bjorn3
Copy link
Member

bjorn3 commented Aug 9, 2019

struct Unit;
type UnitAlias = Unit;

fn new_unit_alias() -> UnitAlias { Unit }

let a = new_unit_alias();
let b: Unit = a;

Should a be shown here as having type Unit or UnitAlias? When looking at the initializer first, you would see UnitAlias, while looking at the usage first, you would see Unit. And if Unit and UnitAlias are swapped?

@flodiebold
Copy link
Member

I think a should be shown as UnitAlias (but b as Unit). (Technically, when determining the type of a, the type annotation is only used to guide inference and unified with the actual inferred type in the end, so we would just have to take care of labeling the correct type as the 'expected' one, which is also important for diagnostics anyway.)

Maybe we would need a setting or an option to show 'normalized' types though.

@bjorn3
Copy link
Member

bjorn3 commented Aug 9, 2019

@flodiebold 👍 for both.

@matklad
Copy link
Member

matklad commented Aug 9, 2019

@flodiebold yeah, doing this for display is centrally both wrong and insufficient (there are cases where you need to guess aliases back anyway).

However, fixing

type List<T> = (T, Box<Option<List<T>>>);

seems like a good idea, if that's actually possible (don't know what it takes to unify (T, Box<Option<List<T>>>) and (T, Box<Option<List<(T, Box<Option<List>>)>>>)`).

Also, this seems like a really good question for type-inference libraryfication, and I hope we'll start on than after nikomatsakis is back.

@umanwizard
Copy link
Contributor Author

umanwizard commented Aug 9, 2019

I completely agree that changing the type system for formatting purposes is wrong.

Actually I think there are two separate issues here, one about typedefs and one about paths. To state the second one more clearly, what if we just had this:

struct Foo {
    inner: blah::Foo,
}

then how should we display the type of inner ?

@umanwizard
Copy link
Contributor Author

umanwizard commented Aug 9, 2019

This is a bit of a tangent, but is anyone aware of what heuristics Clang uses in its error messages? C++ std lib types can be horrible (pages and pages of template crap), but Clang diagnostics seem to usually choose a spelling that is reasonably easy to understand.

@bluss
Copy link
Member

bluss commented Apr 28, 2020

It would be so cool if type aliases could be preserved in the user displayed types, at least to some degree.

In the following example/screenshot, the function returns the type Array3<f64> which is a type alias for ArrayBase<OwnedRepr<f64>, Ix3> which contains a type alias, so it's really ArrayBase<OwnedRepr<f64>, Dim<[usize; 3]>>. And I've heard of many crates that have worse types 😅

Using type aliases would help users see through the too-complex types, and help them write better code too (use the type aliases). The dimensionality information - the 3 - is not visible in the abbreviated full type.

image

@lnicola lnicola added S-actionable Someone could pick this issue up and work on it right now A-ty type system / type inference / traits / method resolution labels Jan 25, 2021
@nbigaouette
Copy link

uom types are quite long. Say for example a Length is a type alias, containing a type alias (Dimension) containing other type alias...

If I don't limit the length of inlays types (rust-analyzer.inlayHints.maxLength), I get extremely long inlays of multiple time my screen size:

Capture d’écran, le 2021-03-26 à 20 26 02

In that project I define type aliases for vectors of uom quantities. I would like to see those type aliases used as inlays and not the full length exact type.

Thanks!

@SomeoneToIgnore
Copy link
Contributor

Another thread on this regard: https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Type.20aliases.20in.20inlay.20hints that makes me reconsider the labels we have on this one.

@SomeoneToIgnore SomeoneToIgnore added S-unactionable Issue requires feedback, design decisions or is blocked on other work and removed S-actionable Someone could pick this issue up and work on it right now labels May 7, 2021
@bluss
Copy link
Member

bluss commented May 10, 2021

While a hard problem, I think the win for users can be immense. Does unactionable mean that we can still work on trying to define a solvable variant of the problem?

@flodiebold
Copy link
Member

Unactionable doesn't mean anyone is prevented from working on it in any way, just that we don't see a direct way of making progress on it right now.

@weiznich
Copy link
Contributor

To just give a example involving diesel where rust-analyzer shows private types as type hint:

#[macro_use] extern crate diesel; // 1.4.6
use diesel::prelude::*;

table! {
    users {
        id -> Integer,
        name -> Text,
    }
}

fn test() {
    let f = users::table.filter(users::name.eq("bar"));
}

which shows SelectStatement<table, DefaultSelectClause, NoDistinctClause, WhereClause<{unknown}>> for as type hint for f. The correct type would be Filter<users::table, Eq<users::name, &'static str>> as it is constructed by the corresponding trait method.

@iMplode-nZ
Copy link

Would it be possible to have like a setting #[rust_analyzer::type_alias(X)] on the structs for this purpose?

@HKalbasi
Copy link
Member

What about generally trying to always using type alias for displaying types when they decrease complexity? We can define "complexity" as sum of generic arg counts, e.g. Result<Box<dyn Trait<T>>, Error> has complexity 2+1+1=4, so type Array3<T> = ArrayBase<OwnedRepr<T>, Ix3> is a type alias that decreases complexity, but type c_int = i32 is not. Then we can try to eagerly apply such type aliases when displaying types, even if the original expression doesn't use it.

This doesn't solve the problem completely, but is a step forward with relatively smaller change, and it is something that we may want to keep even when we support type aliases properly.

@Lokathor
Copy link

The type of a c_int should always show as c_int, or perhaps as c_int = i32.

Particularly with FFI code where the type's width can vary by platform, the stable API is often not "you can use i32" here, it's "you can use c_int here, whatever width that is"

@HKalbasi
Copy link
Member

Yes, that wouldn't help in the c_int case. c_int needs proper support for lazy type aliases. (We definitely don't want to show all i32s in world as c_int).

@Lokathor
Copy link

ah, haha, yeah

@Ruhrpottpatriot
Copy link

I personally would like to have at least some notion of type aliases. I recently had Result<(HashMap<String, HashMap<String, String>>, Option<Vec<String>>)> (don't ask why), which made my function signatures so horrible, that even clippy complained and suggested type aliases. Maybe using clippy's heuristic might be an avenue to tackle this?

Another thing I want to mention is the go extension for VSCode, which shows type aliases. I don't know to what extend, but for them it seems to work. Maybe their approach can be a solution?

@iago-lito
Copy link

iago-lito commented Apr 10, 2024

In the hover text (since there is more room there than inlay hints), I would find it very useful to actually display the full aliasing stack:

TypeA
imported here with `use crate::mod1::A as TypeA;`
imported there with `use super::B as A;`
aliased there with `type B = external::LongTypeNameB;`

This is (kind of) a preview what successive goto would do (yet not skipping the use statements).

@damccull
Copy link

damccull commented May 3, 2024

Today I ran into a problem I think is related to this but in a bad way. I used a code action to implement the missing members of actix-session::storage::SessionStore for my struct and when it populated the types it ended up giving me the alias (SessionState) that crate uses internally for HashMap<String, String>. SessionState is private to the crate so I can't use it and ended up confused for a while until I figured out what it really was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution E-unknown It's unclear if the issue is E-hard or E-easy without digging in S-unactionable Issue requires feedback, design decisions or is blocked on other work
Projects
None yet
Development

No branches or pull requests