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

Don't check the unfinished expression before dot as the whole expression #14718

Merged
merged 11 commits into from
Feb 16, 2023

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Feb 8, 2023

A test PR to check what breaks with this change.

Consider an unfinished expression with a dot at the end: x.. In many cases the dot would be ignored and only x expression would be checked. It has bad consequences where the type of x is unified with the type that is expected in the context, despite it's an inner expression in another unfinished one:

let f x =
    if true then
        x. // `x` is inferred to have type `unit`, which is not expected

That unexpected unit type breaks completion for cases where x should infer to have some other type, like string here:

let g (s: string) = ()

let f1 x =
    if true then
        // user wants to type `Length`,
        // but there's no completion, since `x` is mistakenly considered `unit`
        x. 

    // an error about `unit` not being compatible with `string`
    g x

// the same issues:
let f2 x =
    x. // trying to use `x` members from code completion
    g x

With this change both cases work as expected (i.e. x doesn't have any constraints in the first case and code completion suggests string members in the second one).

Another case is applying an argument:

let g (i: int) = ()
let h (s: string) = ()

let f x =
    g x. // `x` is inferred to `int` instead of `string`
    h x // an error

@auduchinok auduchinok requested a review from a team as a code owner February 8, 2023 17:23
@auduchinok
Copy link
Member Author

auduchinok commented Feb 9, 2023

I've removed a few other related cases that prevented a good type checker recovery.

Before, the right side is ignored:

Screenshot 2023-02-09 at 10 20 27

After, the right side is type checked with the unknown type of the missing expression after dot:

Screenshot 2023-02-09 at 10 21 09

@auduchinok
Copy link
Member Author

Please merge #14739 first, and I'll update the tests here afterwards.

@psfinaki
Copy link
Member

@auduchinok there you go :)

@auduchinok
Copy link
Member Author

@psfinaki Thanks!

I've updated parser tests for unfinished expressions (A., A.B.) and added more tests for various incomplete record field cases (the parsing got broken there in the initial implementation, and only Code Completion tests caught it).

It's ready now.

@psfinaki psfinaki merged commit 91af626 into dotnet:main Feb 16, 2023
@auduchinok auduchinok deleted the tc-mkSynDotMissing branch February 16, 2023 10:54
KevinRansom added a commit to KevinRansom/fsharp that referenced this pull request Feb 20, 2023
KevinRansom added a commit to KevinRansom/fsharp that referenced this pull request Feb 20, 2023
kant2002 pushed a commit to kant2002/fsharp that referenced this pull request Apr 1, 2023
…ion (dotnet#14718)

* Don't check the unfinished expression before dot as the whole expression

* More recovery

* tmp

* Fix record field completion

* Fantomas

* Update surface area

* Fix parsing

* Space

* Fantomas

* Fix tests, add tests for record fields

* Another record field test
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.

5 participants