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

Resolve issue with implicit yields requiring Zero #10556

Merged
merged 4 commits into from
Dec 1, 2020
Merged

Conversation

laenas
Copy link
Contributor

@laenas laenas commented Nov 28, 2020

Related to #8587

I've been wrapping my head around this in the idle hours for a couple of days now and this seems like it should resolve the bug in question. But I would appreciate eyes on it, and will with great joy take guidance on more elegant means of solving this.

Here's the low-down on the bug:
Based on the RFC - it's expected that a yielding CE require Zero (see Detailed Design - point 2)

In the code itself, we can see that the behavior makes sense:
CheckComputationExpressions.fs : 1440 states

// "expr;" in final position is treated as { expr; zero }

And thus, relative to the samples in the ticket - the final 3, a raw expression, would be followed by a zero which would somewhat necessitate the need for that Zero method to exist.

The resolution for this that I have found is to do a safety-check with regards to how we handle ImplicitZero - if we're in a context where Implicit Yielding is enabled, then rather than erroring out if the builder lacks Zero, we can first check to see whether or not we're in an Implicit Yielding situation and therefore, if we are, rely on the translation context provided to handle the zero intelligently.

I've considered the placement of this logic - but cannot identify a better place to shim it in (without affecting more of the surrounding code than I feel comfortable with) than in tryTrans - while this results in a duplicate check around the enableImplicitYield value (now in tryTrans and then nearly immediately afterwards in in translatedCtxt callback defined at :1452) - this seems minor.

My intuition is that there's a cleaner way to handle this, but it may require more significant modifications to the surrounding behaviors here than what I have made here and, as such, I hesitate to do so without further insights.

@cartermp
Copy link
Contributor

Since this would require conformance testing, here is a good location:

https://github.com/dotnet/fsharp/tree/20dec2af41d075a71a04f92c15a09a9a38c4752f/tests/FSharp.Compiler.ComponentTests/Language

Sadly tests are hard to make sense out of as various conformance stuff is strewn about in different projects and suites.

I would add these for now:

  • Test with implicit yield with no zero - none defined in the builder - expected pass
  • Test with implicit yield with a zero - none defined in the builder - expected fail
  • Test with implicit yield with a zero - zero defined in the builder - expected pass

Note that you can write the tests like so:

[<Fact>]
let ``test-name``() =
     FSharp """
// the F# source code
    """
    |> compile
    |> shouldSucceed

or (perhaps preferred, up to you)

[<Fact>]
let ``test-name``() =
     FSharp """
// the F# source code
    """
    |> typecheck
    |> shouldSucceed

or

[<Fact>]
let ``test-name``() =
     FSharp """
// the F# source code
    """
    |> typecheck
    |> shouldFail
    |> withDiagnostics [ (Error 0, Line 0, Col 0, Line 0, Col 0, "the-message" ]

Where the error is an error code, and the line/col numbers correspond to start/end ranges for the error, and the string message is the compiler message.

alternatively, the withDiganostics could be elided, but I think we'd prefer if they were included

@cartermp
Copy link
Contributor

From here it'll be easier to iterate on edge cases if you think of them

@laenas
Copy link
Contributor Author

laenas commented Nov 28, 2020

Perfect - I'll get some tests wrapped around this and update the PR!

@laenas
Copy link
Contributor Author

laenas commented Nov 29, 2020

Beyond the three obvious cases, I also added in the check for the implicit zeroing that occurs with if/else - if that's overkill or redundant (as I could see it being more a matter for how the parser generates the synexpr tree), just let me know.

@laenas laenas marked this pull request as ready for review November 30, 2020 07:53
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

Yes, this is a good change, thank you

@cartermp cartermp merged commit 77bcbe7 into dotnet:main Dec 1, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
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.

3 participants