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

Add new Units type to ease UoM conversions #17518

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

roboz0r
Copy link

@roboz0r roboz0r commented Aug 10, 2024

Description

Creates a new Units type to meet some of the goals in fsharp/fslang-suggestions#892

The type adds Units.Add, Units.Remove, and Units.Cast methods for supported primitive types to simplify UoM conversions. It also adds Units.Add*, Units.Remove*, and Units.Cast* e.g. Units.AddArray to perform UoM conversions on common collection types without allocations.

I haven't added any tests as the code doesn't actually do anything, it's just a wrapper over retype. Happy to add anything you think would be helpful.

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated

Copy link
Contributor

github-actions bot commented Aug 10, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/9.0.100.md

@vzarytovskii
Copy link
Member

vzarytovskii commented Aug 11, 2024

Few questions:

  1. What bothers me is that this is not how we typically add functions to fsharp core - a type with static methods with a bunch of overloads
  2. I think it contradicts a bit with what's approved in the suggestion - what functions and in which part of corelib.
  3. It is missing a bunch of tests and documentation
  4. What's the difference in assembly size before and after the addition?

I think before the implementation we need to start with RFC, and once that is approved and discussed, we can proceed with implementation

@roboz0r
Copy link
Author

roboz0r commented Aug 11, 2024

  1. Agree it's not typical for fsharp core code but I found the ergonomics and discoverability much nicer to have 15 (primitive + 4 collections * 3) methods with 13 overloads than having 195 differently named methods.
  • The naming and overall API are open to discussion but I felt it better to put out a concrete implementation to serve as a starting point for discussion.
  1. The API design was inspired by Ease conversion between Units of Measure (UoM) and undecorated numerals and simplify casting fsharp/fslang-suggestions#892 (comment) the most recent comment in the that was received broadly positively from the few thumbs up. Agree that it is somewhat inconsistent with the initial description but I tried to synthesize with all the comments while avoiding the generic conversion which would require a language change.
  • If you feel it would be better placed in LanguagePrimitives then that's an easy change to make, though as a noob I always felt the LanguagePrimitives module was intended as compiler internal helpers
  1. Definitely still need to add the documentation. I intentionally held off on this, anticipating that the API specific were likely to change.
  • What did you have in mind for the tests? I don't think there's any logic to test so would it just be a series of tests with explicit type annotations and if they compile then it's good?
  1. Comparing the resulting size of FSharp.Core.dll (built with ./Build.cmd -c Release) is 2,250KB to 2,281KB

@vzarytovskii
Copy link
Member

I think we should still start with rfc for it, so everyone agrees on specifics.

@roboz0r
Copy link
Author

roboz0r commented Aug 11, 2024

I put together a draft RFC here fsharp/fslang-design#784 is there something else I missed?

@vzarytovskii
Copy link
Member

I put together a draft RFC here fsharp/fslang-design#784 is there something else I missed?

That looks like a good start, now we need some comments on it. I'll probably be also good to have all types next to examples so people get a good understanding how many methods/functions are added

@T-Gro
Copy link
Member

T-Gro commented Aug 14, 2024

@roboz0r : We have dicussed this addition and have an idea on how to bring the same functionality with a new compiler intrinsic, instead of adding many new functions to FSharp.Core.
Vlad will ping you on Discord on how this could work.

This would especially mean a lot smaller size footprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

3 participants