Skip to content

Conversation

@vasily-kirichenko
Copy link
Contributor

This fixes #3574 and #3576

1

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks good.

@vasily-kirichenko
Copy link
Contributor Author

wait

@vasily-kirichenko
Copy link
Contributor Author

vasily-kirichenko commented Sep 14, 2017

OK, it's really great :) (I tried to use

match adhoctDotSearchAccessible with 
    // If the type is already known, we should not try to lookup a record field
    | Exception _ when isRecdTy ncenv.g typ -> 

but it broke everything).

@vasily-kirichenko
Copy link
Contributor Author

@dotnet-bot test this please

@KevinRansom
Copy link
Contributor

@vasily-kirichenko breaking everything is a small price to pay for It's great ....

@vasily-kirichenko
Copy link
Contributor Author

The current version seems to work OK.

I'm not sure if this is the right solution because Don suggested to use isNominalTy typ || isRecordTy #3576 (comment)

@dsyme
Copy link
Contributor

dsyme commented Sep 14, 2017

@vasily-kirichenko As I explained in #3576 (comment) "isRecordTy" is not enough. The type can be a type variable too (e.g. isTyparTy, or, not isNominalTy is enough)

@dsyme
Copy link
Contributor

dsyme commented Sep 14, 2017

@vasily-kirichenko Yes, what you have now should be enough

@cartermp
Copy link
Contributor

@dotnet-bot test Windows_NT Release_ci_part2 Build please

@vasily-kirichenko
Copy link
Contributor Author

it's ready

// If the type is already known, we should not try to lookup a record field
if isAppTy ncenv.g typ then
NoResultsOrUsefulErrors
else match lid with
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: this is odd code formatting for the compiler. We would normally use

    if X then 
        Y
    else
        match Z with 

or

    if X then Y else
    match Z with 

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

I will mark as approved, but we should normally always add a test for this kind of change. Add it in tests\fsharp\typecheck\sigs

@vasily-kirichenko
Copy link
Contributor Author

Done.

@vasily-kirichenko
Copy link
Contributor Author

@dotnet-bot test this please

@KevinRansom KevinRansom merged commit 5f122c3 into dotnet:master Sep 17, 2017
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.

Record fields are wrongly resolved on unrelated symbols

5 participants