SynType with single slash and type

[edgarfgp]

Pair programming session with @nojaf
First attempt to fix #13348

[nojaf]

The idea we are going to is to split up SynType.Tuple into actual tuples and measurement types.

[<Measure>]
type Foo = x * y / z // SynType.Division

type RealTuple = x * y * z // SynType.Tuple

SynType.Division is a working name, other suggestions are welcome.

There are some real distinctions between the two:

• Tuple can be a struct, Division not.
• Division can be a mix of both * and /
• Division can have a single type element (with leading /)
• Tuple only has * between the type elements

The helper functions mkTupleOrDivide and mkDivideWithLeadingSlash will need to create the right SynType based on the content.

The range of the SynType can be determined by:

let range =
    (leadingType.Range, elementTypes) ||> List.fold (fun acc (_, t) -> unionRanges acc t.Range)

This will combine the range of all types.

Be aware that there is a bit of a twist to this PR so far:
https://github.com/dotnet/fsharp/blob/cc98f840d0803ce4e3205c6323f62c061b20fc55/src/Compiler/pars.fsy#L5089-L5098

tupleOrQuotTypeElements will return a list of bool * SynType, where if you look at the last rule:

  | appType %prec prec_tuptyptail_prefix 
    { [(false, $1)] }

There is no * or / involved and you still get false, SynType an entry.
So for the last element of that list, you should interpret it as only the SynType.
The other elements in the list are SynType and what became before it.
The isStar parameter in mkTupleOrDivide will contain what came after the leadingType.

Maybe for SynType.Division you want to model it as:

| Division of leadingType: SynType option * elementTypes: (bool * SynType list) * range: range

Where the bool in bool * SynType always tells us what came before the SynType.

Examples

[<Measure>] type Foo = x / y
[<Measure>] type Bar = a * b / c
[<Measure>] type Bar = / second

leads to

Division (x, [(false, y)])
Division (a, [(true, b); (false, c)])
Division (None, [(false, second])

This is just a suggestion, but I hope you see where I'm going with this.

Unit tests will need to be added to https://github.com/dotnet/fsharp/blob/0c5671f3b87e3fe076344bbf8997aa6587e582ff/tests/service/Symbols.fs to make sure we covered all the grammar rules.

In the type checking, you will need to split up

fsharp/src/Compiler/Checking/CheckExpressions.fs

Lines 4642 to 4659 in 0c5671f

	and TcTupleType kindOpt cenv newOk checkConstraints occ env tpenv isStruct args m =
	let tupInfo = mkTupInfo isStruct
	if isStruct then
	let argsR,tpenv = TcTypesAsTuple cenv newOk checkConstraints occ env tpenv args m
	TType_tuple(tupInfo, argsR), tpenv
	else
	let isMeasure =
	match kindOpt with
	| Some TyparKind.Measure -> true
	| None -> List.exists (fun (isquot,_) -> isquot) args | _ -> false
	if isMeasure then
	let ms,tpenv = TcMeasuresAsTuple cenv newOk checkConstraints occ env tpenv args m
	TType_measure ms,tpenv
	else
	let argsR,tpenv = TcTypesAsTuple cenv newOk checkConstraints occ env tpenv args m
	TType_tuple(tupInfo, argsR), tpenv

We basically already from the AST is the type is a measure or a tuple.

For the Service walking you will basically need to update the code and do the same thing for Tuple and Division.

The best of luck @edgarfgp.
Feel free to reach out if you have any questions.

[nojaf]

Oh, and in the case of [<Measure>] type Bar = / second, the helper function mkDivideWithLeadingSlash would also need the range of the leading slash to be included in the range of Division.

It is currently present in the range of SynType.Tuple.

You can get this range by changing:
| INFIX_STAR_DIV_MOD_OP tupleOrQuotTypeElements

to

  | INFIX_STAR_DIV_MOD_OP tupleOrQuotTypeElements
    { if $1 <> "/" then reportParseErrorAt (rhs parseState 1) (FSComp.SR.parsUnexpectedInfixOperator());
      let mSlash = rhs parseState 1
      mkDivideWithLeadingSlash mSlash $2 }

val mkDivideWithLeadingSlash: leadingSlash: range -> elementTypes: (bool * SynType) list -> SynType

Calculate the range in a similar fashion as in mkTupleOrDivide.

[edgarfgp]

Thank you @nojaf for the detailed explanation :) .

[auduchinok]

Division can be a mix of both * and /

This sounds not very good to me. The type checker and various features now would need to deal with both Tuple and Divide cases for checking measure type, as they still both can represent these type math cases. It means we can't really distinguish these two cases, so adding the second case looks a bit unneeded, as by simplifying one part (measure types containing both / and *) it makes it more difficult for others cases (types with * only).

Another approach we could consider: what if we kept the types inside Tuple case and only kept the separators info? It would be easy to see if each separator is * or /. The remaining case with a single type element could be encoded with either a separate SynType case or by allowing Tuple to have a single element and to use some active pattern in the type checker.

[nojaf]

I was unaware that a measure type with only * was allowed.

[<Measure>] type Y
[<Measure>] type Z
[<Measure>] type X = Y * Z

is allowed, so this brings us back to the drawing board @edgarfgp.
Eugene has a point, we should keep all in the information inside SynType.Tuple.

We can still, however, keep the changes in pars.fsy and deal with everything inside the ParserHelper.

We do need still need a change I think to somehow deal with / second inside
| Tuple of isStruct: bool * elementTypes: (bool * SynType) list * range: range

Maybe changing elementTypes to bool option * SynType where bool option is the leading / or *. Which the first element would typically not have unless it is our edge-case.

[edgarfgp]

@nojaf Will make the changes this evening :) .

[edgarfgp]

Maybe changing elementTypes to bool option * SynType where bool option is the leading / or *. Which the first element would typically not have unless it is our edge-case.

Yeah that sounds the only way that we can capture our edge-case without changed signature too much

[smoothdeveloper]

SynType.Division is a working name, other suggestions are welcome.

Wonder if it should be explicit about usage for units-of-measure, maybe MeasureDivision?

In the current change, what is the meaning of turning bool into bool option for elementTypes member of SynType.Tuple? should SynType.Tuple be altered at all?

Taking @nojaf suggestion example:

[<Measure>] type Foo = x / y
[<Measure>] type Bar = a * b / c
[<Measure>] type Bar = / second

suggestion:

MeasureDivision (Some x, [(Slash, y)])
MeasureDivision (Some a, [(Star, b); (Slash, c)])
MeasureDivision (None, [(Slash, second])

Maybe being more explicit about what forms a division: Star and Slash, rather than a boolean will pay off?

Maybe it is possible to better refactor to make the Power part of the "grammar" and refactor the currently separate cases into a common MeasureFormula in SynType(

fsharp/src/Compiler/SyntaxTree/SyntaxTree.fsi

Lines 489 to 492 in 0c5671f

	| MeasureDivide of dividend: SynType * divisor: SynType * range: range
	/// F# syntax: for units of measure e.g. m^3, kg^1/2
	| MeasurePower of baseMeasure: SynType * exponent: SynRationalConst * range: range

)

This kind of change, maybe the right time to make it more consistent, alas I am not familiar with how to better model the syntax tree, especially if this is to also contribute to error recovery around faulty measure declarations, I understand it is a can of worms 🙂.

related: #11481

PS: Kudos to @nojaf for pairing with @edgarfgp and congrats to @edgarfgp for tackling parser related issues!

[nojaf]

@smoothdeveloper as Eugene mentioned, the SynType.Division idea won't work.
SynType only captures the right-hand side of the type definition:

[<Measure>] type X = Y * Z
type MyTuple = int * string

In both cases, the right-hand side is SynType.Tuple. The Measure attribute makes it a measure but from a SynType point of view, both contain the same information of what was written.

So changing the semantics from Star to Power doesn't make sense for a regular tuple.
As long as you don't know this is a measure I'd stick to the token names that were found.

[smoothdeveloper]

Got it, overlooked that, thanks @nojaf!

Then, TupleOrMeasureExpression would make it explicit, keeping the token names from the lexer, and I assume in type checking stage, having active patterns to disambiguate.

The pretty printing also shows interesting stuff:

[<Measure>] type x
[<Measure>] type y
[<Measure>] type z
[<Measure>] type A = x * y / z
[<Measure>] type B = x * z / y * z
[<Measure>] type C = x y / z 

outputing:

[<Measure>]
type A = x y/z
[<Measure>]
type B = x z ^ 2/y
[<Measure>]
type C = x y/z

it seems one can omit the * altogether (implied as default composition), and that it performs algebra (to remind me of operator precedence...), in order to do type equivalence properly.

Really interesting area.

[nojaf]

@edgarfgp is it important that we still include the firstType into the existing code.
I think here we need something like firstType :: (List.map snd elements) |> List.tryPick (traverseSynType path) Write Preview.

[edgarfgp]

I had some time today and I made a change based on your suggestion :)

[edgarfgp]

There seems to be a unit test failing but they all pass locally using sh build.sh --test -c Release. Will try rerunning the CI

[edgarfgp]

@dsyme Can we have a review please ?

[nojaf]

I would rename this to mkSynTypeTuple, we already moved away from the divide thing.

[edgarfgp]

Updated. Thanks for the comment :)

[dsyme]

Please call this SynTupleTypeSegment

[nojaf]

Done, thanks for the review @dsyme!

[dsyme]

Just one small request for change