Operator not part of previous block but last line when it follows a match expression or if/else block, caused by ambigious undentation rules

[abelbraaksma]

I have a |>> function defined simply as (|>>) x f = x |> f |> ignore; x, which I use with logging, however, I notice that occasionally it doesn't get executed.

I have difficulty getting a small repro, though it reproduces in several ways in a larger construct.

Repro steps

Not really a repro, but when I use it as follows (though with a different DU), the problem arises.

let inline (|>>) x f = x |> f |> ignore; x

let test1 x =
    match x with
    | Some y -> y
    | None -> 42
    |>> fun x -> Console.WriteLine (sprintf "Found %i" x)

let test2 x =
    match x with
    | Some _ -> 88
    | None -> 44
    |>> fun x -> Console.WriteLine (sprintf "Found again %i" x)  // sometimes not hit

Expected behavior

The inline function should always execute. The actual function does return the expected value, and no compile errors are seen.

So far, it only seems to happen when used after a match expression. When I write the |>> on each matching case, it hits just fine. When I write it at the bottom of the match expression, as above, it sometimes hits (on some code paths, and then always), and sometimes doesn't (different code path, and then never).

Actual behavior

In some cases it just doesn't hit. When I rewrite the statement in a different way, it hits.

Known workarounds

I can rewrite the code as follows, and then it does not hit:

let test2 x =
    let result = 
        match x with
        | Some _ -> 88
        | None -> 44

    result
    |>> fun x -> Console.WriteLine (sprintf "Found again %i" x)

Related information

I know it is not very useful reporting something without a small enough repro, but I am wondering if something like this has been seen before. If not, I can spend the time to try to come up with a small repro by extracting the portions from my main project and see if it repros.

Note that it doesn't matter whether I use Debug or Release builds.

[abelbraaksma]

I found another workaround that hints at what is going wrong. If I outdent the cases an extra time, the operator is always hit:

let test2 x =
    match x with
        | Some _ -> 88
        | None -> 44
    |>> fun x -> Console.WriteLine (sprintf "Found again %i" x)  // sometimes not hit

I also noticed that the last case printed (that is, hit the operator), but other cases didn't. My actual offending code looks like this:

match sequenceType with
| SequenceType.EmptySequence ->
    // The "empty-sequence()" sequence type is mapped to the empty type.
    TI.Empty

| SequenceType.ItemType (itemType) ->
    itemType
    |> ConvertItemTypeToXdmType ctx
    |> TypeInfo.createOne

| SequenceType.ItemTypeWithOccurrenceIndicator (itemType, occurrenceIndicator) ->
    let occInd =
        match occurrenceIndicator with
        | OccurrenceIndicator.Plus -> TypeInfo.combineTInfoWithCountable InfoOneOrMore
        | OccurrenceIndicator.Star -> TypeInfo.combineTInfoWithCountable InfoZeroOrMore
        | OccurrenceIndicator.QMark -> TypeInfo.combineTInfoWithCountable InfoZeroOrOne
    occInd (ConvertItemTypeToXdmType ctx itemType)

|>> fun x -> dbg.Log("Converted sequence type '%O' into TI '%O'", sequenceType, x)  // only hit for the last case

And when written like this, it succeeds always:

match sequenceType with
    | SequenceType.EmptySequence ->
        // The "empty-sequence()" sequence type is mapped to the empty type.
        TI.Empty

    | SequenceType.ItemType (itemType) ->
        itemType
        |> ConvertItemTypeToXdmType ctx
        |> TypeInfo.createOne

    | SequenceType.ItemTypeWithOccurrenceIndicator (itemType, occurrenceIndicator) ->
        let occInd =
            match occurrenceIndicator with
            | OccurrenceIndicator.Plus -> TypeInfo.combineTInfoWithCountable InfoOneOrMore
            | OccurrenceIndicator.Star -> TypeInfo.combineTInfoWithCountable InfoZeroOrMore
            | OccurrenceIndicator.QMark -> TypeInfo.combineTInfoWithCountable InfoZeroOrOne
        occInd (ConvertItemTypeToXdmType ctx itemType)

|>> fun x -> dbg.Log("Converted sequence type '%O' into TI '%O'", sequenceType, x)

I don't see anything inherently wrong with the original code (in fact, I use this coding pattern in a bunch of places), so it seems that the indent scoping rules go wrong here for some reason, letting the compiler think the |>> piping belongs to the last case only.

[zpodlovics]

Note: the test1 have int option -> int type while the test2 have a' option -> int type. Otherwise check and compare the generated IL because the inlining may messed up the pattern matching codegen in some cases.

[TIHan]

We need a small repro for this. Also, the indentation shouldn't change the semantics (I would be very surprised).

[majocha]

@abelbraaksma

I also noticed that the last case printed (that is, hit the operator), but other cases didn't. My actual offending code looks like this:

match sequenceType with
| SequenceType.EmptySequence ->
    // The "empty-sequence()" sequence type is mapped to the empty type.
    TI.Empty

| SequenceType.ItemType (itemType) ->
    itemType
    |> ConvertItemTypeToXdmType ctx
    |> TypeInfo.createOne

| SequenceType.ItemTypeWithOccurrenceIndicator (itemType, occurrenceIndicator) ->
    let occInd =
        match occurrenceIndicator with
        | OccurrenceIndicator.Plus -> TypeInfo.combineTInfoWithCountable InfoOneOrMore
        | OccurrenceIndicator.Star -> TypeInfo.combineTInfoWithCountable InfoZeroOrMore
        | OccurrenceIndicator.QMark -> TypeInfo.combineTInfoWithCountable InfoZeroOrOne
    occInd (ConvertItemTypeToXdmType ctx itemType)

|>> fun x -> dbg.Log("Converted sequence type '%O' into TI '%O'", sequenceType, x)  // only hit for the last case

I believe this happens because of the infix operator indentation rule. There is ambiguity here. The last line can be interpreted as part of the match result expression with "dedented" infix operator and this interpretation apparently takes precedence.

[majocha]

@TIHan This is an unfortunate feature, not a bug. Simple way to trigger this:

Works:

    let f v =
        match v with
        | Some i -> [1..i]
        | None -> []
        |> Seq.sum

Errors:

    let f v =
        match v with
        | Some i -> [1..i]
        | None -> 
           []
        |> Seq.sum

The alignment of last 2 lines here is the key.

[TIHan]

I was referring to when you try indenting like this:

let f v =
    match v with
        | Some i -> [1..i]
        | None -> []
    |> Seq.sum

is the same as:

let f v =
    match v with
    | Some i -> [1..i]
    | None -> []
    |> Seq.sum

[TIHan]

However, there is something interesting going on here:

let compiles v =
    match v with
    | Some i -> [1..i]
    | None -> 
        []
    |> Seq.sum

let not_compiles v =
    match v with
    | Some i -> [1..i]
    | None -> 
       []
    |> Seq.sum

These two are different, which is kinda of surprising. The one that doesn't compile has one less space and it's changing semantics. This might be what is happening to @abelbraaksma.

[majocha]

@TIHan yep it's the infix token indentation. It is in the language spec but I can't find it mentioned in the docs.

It can bite people on rare occasions like @abelbraaksma 's when you have 3 characters long operator and 4 spaces indent.

[TIHan]

@majocha you are right. Indenting does make a difference in this case because we get more more spaces. It's starting to make sense to me now. :)

Though, it's unfortunate that this can happen.

@abelbraaksma can you see if it's the spacing issue for you? If it is, I will close this issue.

Thank you @majocha for looking into this as well.

[abelbraaksma]

The last line can be interpreted as part of the match result expression with "dedented" infix operator and this interpretation apparently takes precedence.

@majocha, keen observation! I didn't think of that, but it seems to make sense (albeit not what I'd expect). Since the |>> operator doesn't change the result type, it doesn't lead to compile errors. But then the line should be hit only when the last DU case is hit (you seem to imply it should be executed after and with the last line starting with occInd).

I rarely use the undenting in practice, perhaps only to align brackets or parens. I will experiment with a variant of the operator (i.e., one that changes the type), which, if you are right, should then lead to compile erros, as @TIHan also showed.

Though, if this is the case, shouldn't we classify it as a bug nonetheless? It seems to me that vertically aligned lines should have higher precedence over non-aligned lines, whether they start with an operator or not. Or at least raise a severe warning.

[TIHan]

Thanks @abelbraaksma .

If it is the case, we unfortunately can't classify it as a bug since it is intended design. If we were to change how this worked, then it would be a breaking change; one that probably wouldn't outweigh the benefits.

[abelbraaksma]

@TIHan, I just hit this again, this time with FParsec, which has many operators that equal or exceed the standard indentation size of 4 (.>>., <??> etc, the latter also not changing the type). The issue was an if-statement, something like:

if foo then skip '.'
else skip '/'
<??> "lookahead_dot"   // sets label for parser, but here only on parser from the 
                       // else-block, highly unpredictable and impossible to detect

I think that, since changing this behavior is backwards-incompatible, we should opt for a warning of some sort, perhaps at lowest level. Something like:

Warning FS9999: Operator undentation ambiguity detected for operator "<??>" causes it to belong to the previous line, instead of the previous block. To avoid this warning, indent the operator to the same indentation level as the previous line, or outdent it to have it belong to the block instead.

I don't know how simple or tricky making such a warning is, but it could certainly help avoiding serious programming mistakes that are notoriously difficult to spot.

[abelbraaksma]

I've created a language suggestion for this here: fsharp/fslang-suggestions#806

[smoothdeveloper]

If I'm not mistaken, this is related: #1019

Thanks for making the warning suggestion.

[abelbraaksma]

@smoothdeveloper, yes, it is. I will add a reference to the language suggestion, thanks.

[dsyme]

This is currently by design. I've marked the language suggestion to emit a warning as "approved in principle".