Skip to content

Conversation

@forki
Copy link
Contributor

@forki forki commented May 3, 2016

This is early work for #1127 and intended for first feedback round:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we want to undo the results of this new "solve" operation (regardless of whether it succeeds or not, using CollectThenUndo). We don't want the results of inference to be altered by giving better errors.

@forki forki force-pushed the downcast branch 2 times, most recently from 41c7a66 to f15b608 Compare May 9, 2016 09:21
@forki
Copy link
Contributor Author

forki commented May 9, 2016

image

@forki forki force-pushed the downcast branch 2 times, most recently from 9e37861 to 9691a36 Compare May 9, 2016 11:55
@isaacabraham
Copy link
Contributor

Just as a side question - why does the compiler give a warning only to then immediately afterwards give an error?

@forki
Copy link
Contributor Author

forki commented May 9, 2016

I think this is an artifact of using fsi. @dsyme can you say more?
On May 9, 2016 2:04 PM, "Isaac Abraham" [email protected] wrote:

Just as a side question - why does the compiler give a warning only to
then immediately afterwards give an error?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1149 (comment)

@isaacabraham
Copy link
Contributor

Ah yes. It doesn't occur through e.g. VSCode.

@forki forki force-pushed the downcast branch 2 times, most recently from 39b9392 to 9bcbb8a Compare May 9, 2016 13:17
@dsyme
Copy link
Contributor

dsyme commented May 9, 2016

Nice. The TcRuntimeTypeTest path is hit on four kinds of syntax:

  • :? ty in patterns
  • :? ty as ident in patterns
  • downcast expr in expressions
  • expr :?> ty

I think the error message change is only suitable for the last case. So that means you should pass in the ContextInfo.RuntimeTypeTest from each of the four callsites of TcRuntimeTypeTest and make sure we have tests for each of the case

Cheers!
Don

@forki
Copy link
Contributor Author

forki commented May 10, 2016

image

@dsyme good catch ;-)

will add corresponding tests and fix that as well.

@forki
Copy link
Contributor Author

forki commented May 10, 2016

image

that's better, right?

@forki forki force-pushed the downcast branch 2 times, most recently from 24b7911 to e6a6ada Compare May 10, 2016 12:25
@forki
Copy link
Contributor Author

forki commented May 10, 2016

tests added for all 4 cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we always gave the error twice. Probably because it is so important to get your types right.
Anyways, I fixed this as well.

@forki forki force-pushed the downcast branch 4 times, most recently from 39df141 to e1db89c Compare May 10, 2016 14:27
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a slightly different version of the hint. It#s not talking about operators

@dsyme dsyme changed the title WIP: Propose to use :> instead of :?> - fixes #1127 Propose to use :> instead of :?> - fixes #1127 May 11, 2016
@dsyme
Copy link
Contributor

dsyme commented May 11, 2016

OK, this looks good to go to me, assuming appveyor CI passes (I know Jenkins is having trouble right now). I've removed the "WIP" .

@forki forki force-pushed the downcast branch 2 times, most recently from 10696e3 to a12b530 Compare May 12, 2016 07:29
@forki
Copy link
Contributor Author

forki commented May 12, 2016

I rebased this.

@forki forki force-pushed the downcast branch 4 times, most recently from 91d304b to 1dfdb7a Compare May 13, 2016 05:56
@forki
Copy link
Contributor Author

forki commented May 13, 2016

I think this is ready and the appveyor error is unrelated

@forki
Copy link
Contributor Author

forki commented May 17, 2016

it's green and ready

@KevinRansom KevinRansom merged commit 7ff89c3 into dotnet:master May 18, 2016
@forki forki deleted the downcast branch September 13, 2016 15:26
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.

5 participants