Skip to content

Conversation

@kerams
Copy link
Contributor

@kerams kerams commented Nov 7, 2022

Implements #14257.

cQvspZ4aZI

@kerams kerams requested a review from a team as a code owner November 7, 2022 20:05
@kerams
Copy link
Contributor Author

kerams commented Nov 8, 2022

Should be good to go unless there's some other way of using DUs that I didn't think of.

psfinaki
psfinaki previously approved these changes Nov 8, 2022
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Kudos for jumping on that, @kerams!

Good testing and optimizations, and also thanks for keeping the original errh "design" - we'll see how long it's going to make sense :)

@psfinaki psfinaki requested a review from a team November 8, 2022 12:03
@psfinaki psfinaki linked an issue Nov 8, 2022 that may be closed by this pull request
@kerams kerams requested a review from psfinaki November 8, 2022 12:57
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Should be good to go unless there's some other way of using DUs that I didn't think of.

@kerams so actually, just before I wanted to reapprove the PR, I opened our own repo with these DU hints on and discovered that there are some places that cause crashes now. The easiest repro is the following:

type X =
    member _.Test() = 42 :: [42; 42]

So yeah we should handle and test this :)

@kerams
Copy link
Contributor Author

kerams commented Nov 8, 2022

Hehe, to be fair it's GetAllArgumentsForFunctionApplicationAtPosition that's failing, not my code :P. Good catch though.

I think Cons needs to be special-cased here and ignored.

@kerams kerams requested a review from psfinaki November 8, 2022 14:21
@psfinaki psfinaki enabled auto-merge (squash) November 8, 2022 15:39
@psfinaki
Copy link
Contributor

psfinaki commented Nov 8, 2022

Thanks again - btw while testing parameter name hints I actually found a bug in type hints, should be also an easy fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Add parameter name hints for discriminated unions

3 participants