[RFC FS-1091] Extend Unsigned Integers to Carry Units of Measure

[pblasucci]

RFC https://github.com/fsharp/fslang-design/blob/main/preview/FS-1091-Extend-Units-of-Measure.md

So, this work is in response to this suggestion: fsharp/fslang-suggestions#901. But, there's no associated RFC. If one is needed, I'd be happy to pull it together. However, at the point, the changes seem complete.

Also, this has only been tested on Linux. And it's my first patch to the compiler. So if there are any issues (I expect two QA tests to fail), or any changes needed, just let me know.

Thanks!

[dnfadmin]

All CLA requirements met.

[cartermp]

Thanks, good work so far. Looks like there are two fsharpqa failures:

E_UnsupportedTypes01

and

E_UnsupportedType01

[pblasucci]

Thanks, @cartermp!

I was actually expecting those two failures. And, now that I've seen them (thus validating my understanding of things), I've submitted a patch. It also updates the error message around supported types.

[abelbraaksma]

Nice work! I made some review comments. In addition, perhaps it's good to add a test with a user-defined type alias, either as a negative test (if it isn't supported) or a positive test, if it is:

This currently fails:

[<Measure>] type mm;
type myint = uint64
type measureint = myint<mm>   // not allowed
let _: measureint = 42UL<mm>

This is valid (at least with int64):

[<Measure>] type mm;
type measureint = uint64<mm>  // allowed
let _: measureint = 42UL<mm>

Same problem can currently be shown to be invalid: float works with a type alias, double does not.

[pblasucci]

Thanks for the thorough review @abelbraaksma. 👍

I will go ahead an make most of the changes you've suggested. But I notice from two or three of them that there is a larger question to be answered (by more than just you and I). Specifically:

What is the full list of numeric types, and type aliases, we want to support with units of measure?

The following table summariezes the current state of things:

CLR Type	F# alias	units?
System.Single	float32	yes
"	single	NO
System.Double	float	yes
"	double	NO
System.Decimal	decimal	yes
System.SByte	sbyte	yes
"	int8	NO
System.Int16	int16	yes
System.Int32	int	yes
"	int32	NO
System.In64	int64	yes
System.Byte	byte	yes
"	uint8	NO
System.UInt16	uint16	yes
System.UInt32	uint	yes
"	uint32	NO
System.UIn64	uint64	yes
System.IntPtr	nativeint	NO
System.UIntPtr	unativeint	NO
System.Numerics.BigInteger	bigint	NO

In terms of effort, I'd have to do some more digging before commenting on bigint and the native integers BigInt is too much work. It should be its own PR. However, native integers and extending the other aliases should be a simple, straight-forward process.

The other issue you raised was around extending units of measure to include custom aliases for numeric types. I'm not opposed to this idea. But I think it's a larger piece of work that would likely requiring touching the parser (whereas extending the existing aliases is mostly done in the type checker).

So, I'll pose the question to @cartermp, @dsyme, and others: which additional types/aliases, if any, do we want to support?

[pblasucci]

Hmm... Seems I don't quite understand how the QA suite works, after all. 🤕

@cartermp is there anyway, in Azure DevOps, to see why the test failed? I can certainly see which test is responsible -- but only that it failed. And, since I can't run the QA suite locally, I'm at a loss for how to proceed.

[abelbraaksma]

The other issue you raised was around extending units of measure to include custom aliases for numeric types

I believe the current aliases are defined in FSharp.Core, meaning that if you can fix them, you automatically fixed it for all aliases.

If that's not the case, I'd argue to just stick to the built in aliases, if these are indeed treated specially in the compiler, lexer already.

From an orthogonality standpoint, it'd be nice if any alias works, of course. But the current pr as it stands is already a great addition.

Suppose it's not trivially doable, we might instead consider a more detailed message, so that users understand the alias is not usable.

[abelbraaksma]

I understand you cannot run the qa test locally? Because there's a lot file that shows the expected outcome vs the actual one, and that file is not available (yet) in the CI output.

I can run it for you in half an hour or so when behind my desk again.

[abelbraaksma]

See #9869

[pblasucci]

Thanks @abelbraaksma! I appreciate any help I can get. Also, no rush... I probably won't be able to jump back into this work for the next several hours.

FYI: the QA suite seems to require the Visual Studio bits (i.e. it won't run on Linux), which is why I can't manage it locally. 🤔 If I get desperate, maybe I'll try the codespace stuff.

[cartermp]

Yeah, FSharpQA suites don't work except for on Windows with some VS bits. It's awful and the first thing we're going to try to migrate to normal xunit tests.

[abelbraaksma]

Ah, from 75babf2 I see that our little effort together has worked! 👍.

[cartermp]

Labeled as needs-rfc. That will be a prerequisite for this to be merged, but it's great to have a working implementation to bounce around!

[pblasucci]

@cartermp RFC ready: fsharp/fslang-design#495

[pblasucci]

@abelbraaksma With respect to adding UoM to custom numeric abbreviations, I looked into "automatically" allowing the abbreviations and... well, it's far beyond my knowledge of the compiler. So, I definitely think it's outside the scope of this Suggestion/RFC/PR.

Additionally, given that there already exists a way to do it (albeit in a slightly more verbose fashion), it feels like a lower priority item (at least for me).

[abelbraaksma]

I understand. Maybe I'll give it a go one of these days. I feel it serves the principle of least surprise to support type aliases, and thinking of it a bit more, it may help improve readability of code (esp with custom type aliases, like type position = uint32, then you can have position<line>, and position<col>, which would actually improve readability of certain parts of the compiler, if it were possible).

But let's do that in an upcoming proposal.

[pblasucci]

So, I'm fairly certain all the guards and tests and such are in place for this now. In other words, unless someone has some feedback/changes, I think it's ready to be moved to the next phase. Thoughts?

[cartermp]

Yeah, I think this is generally good. The tricky bit now is that we're in lockdown mode for F# 5 we may want to hold off on any additions just for the sake of stability. Since this would come in under a preview flag it wouldn't be a part of F# 5, so it's less about introducing something late in the game as it is introducing some churn that could potentially destabilize us.

I'll defer to @KevinRansom on if we want to take this now or later.

[abelbraaksma]

This looks great! Fwiw, it looks like relatively low-risk to still add to the F# 5.0 branch and considering the long-standing wish to have this in for all primitive types, and the cool bug-fix to support all build-in primitive aliases makes one wish/hope this makes the threshold 💯 🙏.

[cartermp]

Update: we're closed down for F# 5 (barring enormous bugs) and we'll have a plannign session soon for the next sprint. I intend to bringing this to the table.

[KevinRansom]

Small naming change, otherwise this is great.

[KevinRansom]

FSharp.Core unit tests are compiled to test as LangeVersion Preview. So this is fine.

[KevinRansom]

@pblasucci Thanks for this it is a great addition to the language.

[KevinRansom]

bugger should have merged it before don got happy chopping up the TypeChecker. I will fix the conflicts and merge tomorrow. This will likely be preview for an age.

[KevinRansom]

@pblasucci, hey I don't have permission to write back to your branch. However, a local merge on your machine, works with out conflict. So I will let you merge it yourself :-)

[pblasucci]

@KevinRansom No worries, sir. 🙂 I've done as you suggested and we're back to green now.

[cartermp]

Thanks @pblasucci!