Skip to content

Conversation

@forki
Copy link
Contributor

@forki forki commented May 12, 2016

This does two things:

  • We don't talk about "infinite types" any more and just talk about unification issues.
  • In the case of return we give advice to use return!

image

@forki forki changed the title Improve error reporting: Recursive async functions WIP: Improve error reporting: Recursive async functions May 12, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

Will misuses of "yield" now report an error about "return"? Test cases for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we distinguish the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh just saw I didn't commit any of my tests. oups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsyme do you know a sample that would create infinite types using yield?!
And I think the code is only triggered from with CE that are not seq

Copy link
Contributor Author

@forki forki May 18, 2016

Choose a reason for hiding this comment

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

@Tarmil gave the following code sample.
image

So indeed we have a false positive. Adding as test case and trying to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/// F# syntax: yield expr 
/// F# syntax: return expr 
/// Computation expressions only
| YieldOrReturn   of (bool * bool) * SynExpr * range

@dsyme should we split that in ast.fs? Maybe in separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

so that's good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I leave
image

for another day ;-)

@dsyme
Copy link
Contributor

dsyme commented May 12, 2016

Awesome. The code looks just fine.

I do still wonder if we should say something more than "types can't be unified", given we know there is a specific reason for this (one type would be included in the other, the types would be infinite etc.).

@forki
Copy link
Contributor Author

forki commented May 12, 2016

yes we can improve on the wording. I just think we should avoid to talk about "infinite types" here. Even if this is the correct term it's probably a term that only very few people know. maybe we could use wording that talks about an "infinite loop while unifying". That is maybe a little less correct but I assume everyone has already seen an infinite loop.

@dsyme
Copy link
Contributor

dsyme commented May 12, 2016

@forki Any idea why AppVeyor is no longer showing in the list of CI builds?

@forki
Copy link
Contributor Author

forki commented May 12, 2016

I think I saw a comment from @otawfik-ms that he removed the appveyor trigger.

@dsyme
Copy link
Contributor

dsyme commented May 12, 2016

@forki Oh, ok.

@otawfik-ms can you confirm?

I don't mind as long as what we have is reliable - I'm just a creature of habit and I'm used to AppVeyor. I haven't learned Jenkins yet, but I'll start.

@otawfik-ms Can you make sure either @forki or myself or both have admin access on Jenkins please? And @KevinRansom too. Thanks.

@OmarTawfik
Copy link
Contributor

@dsyme @forki Jenkins is the main CI server for all dotnet projects now. Roslyn/other dotnet projects will be using it, with much better support for updates, parallel execution, and it is much faster (we are running all tests in CI now).
However, we will keep the appveyor script because a few community members suggested that they still want to run appveyor on their private forks for CI.

@forki forki force-pushed the returnbang branch 6 times, most recently from 3eaa065 to 3d90dee Compare May 18, 2016 06:51
@forki
Copy link
Contributor Author

forki commented May 18, 2016

just for fun, this is what elm gives for something similar:

image

Source

@forki forki force-pushed the returnbang branch 4 times, most recently from 7c087f0 to c0bbad6 Compare May 18, 2016 14:18
@KevinRansom
Copy link
Contributor

ci_part2 failed:

http://dotnet-ci.cloudapp.net/job/Microsoft_visualfsharp/job/master/job/release_ci_part2_windows_nt_prtest/348/consoleText

+++ Warnings (ReturnInsteadOfReturnBang.fs) +++
Microsoft (R) F# Compiler version (private)
Copyright (c) Microsoft Corporation. All Rights Reserved.
(4,32): error FS0001: Type mismatch. Expecting a
    ''a'    
but given a
    'Async<'a>'    
The types ''a' and 'Async<'a>' cannot be unified.

Consider using 'return!' instead of 'return'.

*** The following necessary lines were never matched:
*** \(4,32\):.+error FS0001:.+.+Consider using 'return!' instead of 'return'.*


*** The following necessary lines were incorrectly matched:

Unexpected Compiler Output 
FAIL
+++ Warnings (YieldInsteadOfYieldBang.fs) +++
Microsoft (R) F# Compiler version (private)
Copyright (c) Microsoft Corporation. All Rights Reserved.
(7,30): error FS0001: Type mismatch. Expecting a
    ''a'    
but given a
    ''a list'    
The types ''a' and ''a list' cannot be unified.

Consider using 'yield!' instead of 'yield'.

*** The following necessary lines were never matched:
*** \(7,30\):.+error FS0001:.+.+Consider using 'yield!' instead of 'yield'.*


*** The following necessary lines were incorrectly matched:

Unexpected Compiler Output 
FAIL







Error: 'RunTests.cmd release fsharpqa ' failed

@forki
Copy link
Contributor Author

forki commented May 18, 2016

Yeah I'm trying like crazy to find the correct Regex. I think I tried like
15 times now
On May 18, 2016 7:55 PM, "Kevin Ransom (msft)" [email protected]
wrote:

ci_part2 failed:

http://dotnet-ci.cloudapp.net/job/Microsoft_visualfsharp/job/master/job/release_ci_part2_windows_nt_prtest/348/consoleText

+++ Warnings (ReturnInsteadOfReturnBang.fs) +++
Microsoft (R) F# Compiler version (private)
Copyright (c) Microsoft Corporation. All Rights Reserved.
(4,32): error FS0001: Type mismatch. Expecting a
''a'
but given a
'Async<'a>'
The types ''a' and 'Async<'a>' cannot be unified.

Consider using 'return!' instead of 'return'.

*** The following necessary lines were never matched:
** (4,32):.+error FS0001:.+.+Consider using 'return!' instead of 'return'.

*** The following necessary lines were incorrectly matched:

Unexpected Compiler Output
FAIL
+++ Warnings (YieldInsteadOfYieldBang.fs) +++
Microsoft (R) F# Compiler version (private)
Copyright (c) Microsoft Corporation. All Rights Reserved.
(7,30): error FS0001: Type mismatch. Expecting a
''a'
but given a
''a list'
The types ''a' and ''a list' cannot be unified.

Consider using 'yield!' instead of 'yield'.

*** The following necessary lines were never matched:
** (7,30):.+error FS0001:.+.+Consider using 'yield!' instead of 'yield'.

*** The following necessary lines were incorrectly matched:

Unexpected Compiler Output
FAIL

Error: 'RunTests.cmd release fsharpqa ' failed


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1180 (comment)

@forki
Copy link
Contributor Author

forki commented May 18, 2016

@KevinRansom @enricosada any ideas why it doesn't match?

@forki forki force-pushed the returnbang branch 3 times, most recently from 9e8a92e to 3170fb0 Compare May 19, 2016 10:05
@forki
Copy link
Contributor Author

forki commented May 23, 2016

this is seriously driving me nuts. I can't see the error in my regex.

@enricosada
Copy link
Contributor

try multiple <Expect with single parts

//<Expects status="Error" span="(7,30)" id="FS0001">.Type mismatch. Expecting a.+''a'.+but given a.+''a list'.+The types ''a' and ''a list' cannot be unified. Consider using 'yield!' instead of 'yield'.*</Expects>

also span="(7,30)" is ok? i think the error message has more characters

@forki
Copy link
Contributor Author

forki commented May 23, 2016

That's where it starts. Tbh I only want to check for the "consider" part.
On May 23, 2016 16:46, "Enrico Sada" [email protected] wrote:

try multiple <Expect with single parts

//.Type mismatch. Expecting a.+''a'.+but given a.+''a list'.+The types ''a' and ''a list' cannot be unified. Consider using 'yield!' instead of 'yield'.*

also span="(7,30)" is ok? i think the error message has more characters


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1180 (comment)

@KevinRansom KevinRansom merged commit 8d89887 into dotnet:master May 23, 2016
@KevinRansom
Copy link
Contributor

@forki I took care of the error messages:
4cce423

@forki
Copy link
Contributor Author

forki commented May 23, 2016

Thanks so much Kevin. Didn't understand what was wrong.
On May 23, 2016 10:06 PM, "Kevin Ransom (msft)" [email protected]
wrote:

@forki https://github.com/forki I took care of the error messages:

4cce423
4cce423


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1180 (comment)

@forki forki deleted the returnbang branch May 25, 2016 11:22
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.

Improve error reporting: Recursive async functions

6 participants