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

Raise error when using active pattern in literal binding #10816

Merged
merged 1 commit into from
Dec 30, 2020

Conversation

ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Dec 29, 2020

This PR aims to fix #10816, which noted that one can use an active pattern in a [<Literal>] binding.

Repro:

let (|A|) x = x + 1
let [<Literal>] (A x) = 1

Compiled to:

> let (|A|) x = x + 1;;
val ( |A| ) : x:int -> int

> let [<Literal>] (A x) = 1;;
val x : int = 1

With this PR, this now outputs:

test.fs(2,17): error FS3391: A [<Literal>] declaration cannot use an active pattern for its identifier

This is my very first F# compiler PR (excluding docs), so I'd welcome any feedback.

@@ -9755,6 +9755,9 @@ and TcLetBinding cenv isUse env containerInfo declKind tpenv (synBinds, synBinds
| _ when inlineFlag.MustInline ->
error(Error(FSComp.SR.tcInvalidInlineSpecification(), m))

| TPat_query _ when HasFSharpAttribute cenv.g cenv.g.attrib_LiteralAttribute attrs ->
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'm not sure whether HasFSharpAttribute cenv.g cenv.g.attrib_LiteralAttribute attrs reads well enough. I could add an usesLiteralAttribute binding before this is used, but that would come at the cost of executing that function each time, even when the above pattern matches.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether HasFSharpAttribute cenv.g cenv.g.attrib_LiteralAttribute attrs reads well enough.

It looks similar to many of other checks in the compiler, I guess it should be fine.

@@ -1552,3 +1552,4 @@ forFormatInvalidForInterpolated4,"Interpolated strings used as type IFormattable
3390,xmlDocDuplicateParameter,"This XML comment is invalid: multiple documentation entries for parameter '%s'"
3390,xmlDocUnresolvedCrossReference,"This XML comment is invalid: unresolved cross-reference '%s'"
3390,xmlDocMissingParameter,"This XML comment is incomplete: no documentation for parameter '%s'"
3391,tcLiteralAttributeCannotUseActivePattern,"A [<Literal>] declaration cannot use an active pattern for its identifier"
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've tried looking into re-using an existing error, and there were some candidates. In the end I think it made more sense to introduce a new error message that is very specific to this case.

Copy link
Member

Choose a reason for hiding this comment

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

Separate errors will definitely make it easier on the tooling side, thanks.

@@ -367,6 +367,11 @@
<target state="translated">use! se nedá kombinovat s and!.</target>
<note />
</trans-unit>
<trans-unit id="tcLiteralAttributeCannotUseActivePattern">
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 don't know if these translation files should be checked in too.

Copy link
Contributor

Choose a reason for hiding this comment

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

These do need to be checked in, yes. It's a part of our localization system for VS.

open FSharp.Compiler.SourceCodeServices


module ``Invalid literals`` =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another InvalidNumericLiteralsTests.fs file, but this doesn't really belong there. I then had to decide between creating a test file for this specific case, or introducing a more general file that could contain other invalid literals. I've opted for the latter.

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!


[<Fact>]
let ``Using Active Pattern``() =
CompilerAssert.TypeCheckSingleError
Copy link
Contributor

Choose a reason for hiding this comment

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

N.B. - not required to get this merged, but we do have a different approach that we're slowly moving tests over to use:

FSharp """
// the code
    """
    |> compile
    |> shouldFail
    |> withSingleDiagnostic (Error 123, Line 3, Col 17, Line 3, Col 22, "The message")
    |> ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't know. I just copied from another test. I'll update!

Copy link
Contributor Author

@ErikSchierboom ErikSchierboom Dec 29, 2020

Choose a reason for hiding this comment

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

I've updated to:

    FSharp """
let (|A|) x = x + 1
let [<Literal>] (A x) = 1
    """
    |> typecheck
    |> shouldFail
    |> withSingleDiagnostic (Error 3391, Line 3, Col 17, Line 3, Col 22, "A [<Literal>] declaration cannot use an active pattern for its identifier")

I've changed the compile call to typecheck, to be in line with what other tests were doing and due to it not making the tests pass due to the code missing a module/namespace. Is that okay?

I also noticed that the other test suites seem to omit the ignore call at the end. Do you want me to add it here? It does work without 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.

The new approach looks a lot nicer BTW 👍

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine without the explicit "ignore".

@vzarytovskii vzarytovskii merged commit 94885f5 into dotnet:main Dec 30, 2020
@ErikSchierboom ErikSchierboom deleted the active-pattern-literal branch December 30, 2020 07:22
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.

4 participants