Add error tcActivePatternArgumentCountNotMatch

[ijklam]

Description

Add a new error to improve readability for active pattern case argument count mismatch.

This active pattern case needs %d argument(s) and %d return value, but here has %d argument(s) and %d return value.

QUESTION: what is the best name for the "return value" of an active pattern case?

before:

after:

Fixes # (issue, if applicable)

Checklist

• Test cases added

• Performance benchmarks added in case of performance changes

• Release notes entry updated:

  Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

  Release notes files:

  • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
  • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
  • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

  Information about the release notes entries format can be found in the documentation.
  Example:

  • More inlines for Result module. (PR #16106)
  • Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
    *while! (Language suggestion #1038, PR #14238)

  If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

[ijklam]

Should we allow this?

let (|T2|) a b = ()
match 2 with T2 1 -> ();;

This is an error now

[vzarytovskii]

Should we allow this?

let (|T2|) a b = ()
match 2 with T2 1 -> ();;

This is an error now

I think this shouldn't be explicitly allowed.

[vzarytovskii]

QUESTION: what is the best name for the "return value" of an active pattern case?

I think "return value" works, however "1 return value" in the error message looks a bit awkward.

[T-Gro]

My feedback for the wording:
For parametrized active patterns, the message should say "parameters" when there is a mismatch.
For non-parametrized actived patterns (will be a majority), the 0 parameters/arguments would be better not mentioned at all in the message.

For the return value, I would use the wording:
This active pattern returns 1 value, but the usage here matches 0 values

[ijklam]

What about this?

For parametrized single case active pattern and partial active pattern：

This active pattern requires %d parameter(s) and returns %d value, but the usage here matches %d argument(s) and %d return value.

For else：

This active pattern returns %d value, but the usage here matches %d return value(s).

@vzarytovskii @T-Gro

[T-Gro]

value(s).

That looks better to me.
But anywhere you had "%d value", I would suggest to apply "value(s)".
Otherwise it would just look bad in the case of 0,2 or more.

[psfinaki]

It's a good idea to improve this error. At the same time, as a non-user of active patterns, I got lost even with the new message.
"Return value(s)" sound very misleading to me, how about "captured value(s)"?

[brianrourkeboll]

That is not a partial active pattern. And before this PR, the usage also cannot compile without a (). This PR will not change that.

Right. Although it's arguably somewhat unfortunate that total active patterns differ from partial/Boolean-returning ones in this way.

These were the four messages I originally had in my comment, the third of which sounds like it would be used for partial/Boolean-returning parameterized patterns:

1. This active pattern does not expect any arguments, i.e., it should be used like 'P' instead of 'P x'.
2. This active pattern expects exactly one pattern argument, e.g., 'P x'.
3. This active pattern expects %d expression argument(s), e.g., 'P e1...'.
4. This active pattern expects %d expression argument(s) and a pattern argument, e.g., 'P e1... pat'.

I.e., (3) would be used for:

// Expects: 1 expression argument, 0 pattern arguments.
let (|P|_|) expr2 expr1 = Some P

match expr1 with P -> …
//               ↑
//               This active pattern expects 1 expression argument(s), e.g., 'P e1...'.

match expr1 with P expr2 expr3 -> …
//               ↑
//               This active pattern expects 1 expression argument(s), e.g., 'P e1...'.

[ijklam]

the four situation @brianrourkeboll

[brianrourkeboll]

@Tangent-90

the four situation

I think this is a pretty solid improvement over the status quo.

I guess since we do have the expected number of expression and pattern arguments available to us, we could in theory even offer something like P e1 e2 e3, P e1 e2 pat, etc., in the examples, rather than using e1... as a stand-in for $n$ expressions.

I wonder whether it would be worth doing something like:

// Use a prebuilt expr args string for these that has an example with the actual number of args.
"This active pattern expects %d expression argument(s), e.g., '%s%s'."
"This active pattern expects %d expression argument(s) and a pattern argument, e.g., '%s%s pat'."

and

let fmtExprArgs paramCount =
    let rec loop i (sb: StringBuilder) =
        let cutoff = 10
        if i > paramCount then sb.ToString()
        elif i > cutoff then sb.Append("...").ToString()
        else loop (i + 1) (sb.Append(" e").Append i)

    loop 1 (StringBuilder())

let msg =
    match paramCount, returnCount with
    | 0, 0 -> FSComp.SR.tcActivePatternArgsCountNotMatchNoArgsNoPat(caseName, caseName)
    | 0, _ -> FSComp.SR.tcActivePatternArgsCountNotMatchOnlyPat(caseName)
    | _, 0 -> FSComp.SR.tcActivePatternArgsCountNotMatchArgs(paramCount, fmtExprArgs paramCount, caseName)
    | _, _ -> FSComp.SR.tcActivePatternArgsCountNotMatchArgsAndPat(paramCount, fmtExprArgs paramCount, caseName)

giving

This active pattern expects 2 expression argument(s), e.g., 'P e1 e2'.
This active pattern expects 3 expression argument(s) and a pattern argument, e.g., 'P e1 e2 e3 pat'.

etc.

Or would that be overkill? What do you think @psfinaki?

[psfinaki]

In the topic of diagnostics, it's hard to do an overkill, it's more about perfectionism here now :)

The current state of the art is definitely an improvement compared to the main. "Use 'IsEven' instead of 'IsEven x' probably captures most of the usecases and make the error very clear. The other errors importantly contain the expected number of "things" (arguments, expressions, patterns) and also list all these terms, like hey, there are all these capabilities of active patterns, read the docs if you're interested.

So @Tangent-90 if you're not tired yet, feel free to use Brian's addition. If you are tired, feel free to leave things as they are. I'd just ask to add the tests for the new errors, among other things they can serve as documentation for the time being.

[ijklam]

message updated and test added @brianrourkeboll @psfinaki

[psfinaki]

Good stuff, thanks for this. Active patterns are much more usable now :)

[psfinaki]

Build errors are unrelated, we hope to resolve those soon, sorry.