Skip to content

Conversation

@lnicola
Copy link
Member

@lnicola lnicola commented Mar 31, 2020

Fixes #3796

r? @Veetaha

@lnicola lnicola force-pushed the no-reclit-chaining-hints branch from d5fc772 to 86b0283 Compare March 31, 2020 18:32
@lnicola lnicola force-pushed the no-reclit-chaining-hints branch from 86b0283 to 5ad7fc3 Compare March 31, 2020 18:37
@Veetaha
Copy link
Contributor

Veetaha commented Mar 31, 2020

Fixes #3796 actually*

@lnicola
Copy link
Member Author

lnicola commented Mar 31, 2020

I wonder if there are other expression types we might want to exclude. I think it's fine to leave tuples in.

@Veetaha
Copy link
Contributor

Veetaha commented Mar 31, 2020

Unit structs maybe and the unit type itself? Basically i think the most general heuristic is "whether the text already contains the type name"

@lnicola
Copy link
Member Author

lnicola commented Mar 31, 2020

Good point about those. I'm not sure how to filter them, though, as they show up as a PathExpr and that might exclude too much.

@Veetaha
Copy link
Contributor

Veetaha commented Mar 31, 2020

Hard to say, looks like we can use the type_of_expr() api Here.
Maybe smth like:

if let Some(ra_hir::Adt::Struct(st)) = sema.type_of_expr(expr).as_adt() {
    if matches!(st.variant_data(), ra_hir_def::VariantData::Unit) {

   }
}

@Veetaha
Copy link
Contributor

Veetaha commented Mar 31, 2020

Actually, I think the first approach should mostly be using the ra_hir API and not messing with ast when it comes to smth semantics-related.

@matklad
Copy link
Contributor

matklad commented Apr 1, 2020

r? @SomeoneToIgnore :)

@lnicola
Copy link
Member Author

lnicola commented Apr 1, 2020

PathExpr also includes constants and other stuff. And variant_data is private.

@SomeoneToIgnore
Copy link
Contributor

Will have a look in an hour.

@lnicola lnicola force-pushed the no-reclit-chaining-hints branch from 5ad7fc3 to 7bd1096 Compare April 1, 2020 09:10
@lnicola lnicola changed the title Don't show chaining hints for record literals Don't show chaining hints for record literals and unit structs Apr 1, 2020
return None;
}
if let Some(Adt::Struct(st)) = ty.as_adt() {
if st.fields(sema.db).is_empty() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still not a fan of the extra allocations.

Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot avoid them completely with the current approach to the API, where we avoid excessive reference and lifetimes usage, so I think it's a compromise we have to live with for now.

Copy link
Contributor

@Veetaha Veetaha Apr 1, 2020

Choose a reason for hiding this comment

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

Why not making this a getter like kind -> Union | Record | Tuple. But, here is what I've thought. This will be a false-positive when the expression is not the struct literal, but e.g. some function call that returns a unit or empty-tuple struct. Maybe we lack ra_hir::Expr representation for such cases where we want to determine the kind of expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, added a check for PathExpr.

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

LGTM when the "compilation error" in the test is fixed

@lnicola lnicola force-pushed the no-reclit-chaining-hints branch from 7bd1096 to 10dd3c9 Compare April 1, 2020 09:34
@SomeoneToIgnore
Copy link
Contributor

Actually, I think the first approach should mostly be using the ra_hir API and not messing with ast when it comes to smth semantics-related.

By the way the PR looks now, it does exaclty this, having the main logic check in hir.
I think this PR a good starting point for now, since requires minimal changes to close the issue and gives the food for thoughts about similar heuristics.

Personally, I'm more concerned about the "do not display chain hint" logic spread over 3 places in the file, but that may be addressed later by some other PR.

@lnicola lnicola force-pushed the no-reclit-chaining-hints branch from 10dd3c9 to 70960df Compare April 1, 2020 10:14
@lnicola
Copy link
Member Author

lnicola commented Apr 1, 2020

r? @matklad 😄

@matklad
Copy link
Contributor

matklad commented Apr 1, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 1, 2020

@bors bors bot merged commit a0cc664 into rust-lang:master Apr 1, 2020
@lnicola lnicola deleted the no-reclit-chaining-hints branch April 1, 2020 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add heuristics for struct literal type omission in chaining hints

4 participants