Skip to content

Revert IQuantity.UnitInfo DIM, expose GetUnitInfo() extension on all TFMs#1657

Merged
angularsen merged 2 commits intomasterfrom
claude/revert-iquantity-unitinfo-dim
May 2, 2026
Merged

Revert IQuantity.UnitInfo DIM, expose GetUnitInfo() extension on all TFMs#1657
angularsen merged 2 commits intomasterfrom
claude/revert-iquantity-unitinfo-dim

Conversation

@angularsen
Copy link
Copy Markdown
Owner

@angularsen angularsen commented May 1, 2026

Summary

Reverts the default interface members added in #1649 and exposes GetUnitInfo() as extension methods on all target frameworks instead. The typed overload returns the most specific UnitInfo<TQuantity, TUnit> when overload resolution can infer both type parameters from the receiver.

Rationale

Per @lipchev's review feedback on #1649:

  • Lean interface. The DIM is just a getter over QuantityInfo[UnitKey] — no new state, sets a precedent for further interface bloat (Dimensions etc.).
  • Semantic trap. A custom IQuantity implementor overriding UnitInfo would silently diverge from internal lookups (UnitConverter, UnitAbbreviationsCache, QuantityFormatter) which all go through QuantityInfo, not the new property.
  • Boxing on structs. Calling Mass.Zero.UnitInfo through IQuantity on a struct quantity boxes the receiver per call; concrete quantities don't override the property so the DIM path is the only one available.

The extension method gives the same call-site ergonomics (quantity.GetUnitInfo()) on every TFM with none of these issues.

Overload resolution

Caller has Resolved overload Return type
Length q; GetUnitInfo<TQuantity, TUnit>(IQuantity<TQuantity, TUnit>) UnitInfo<Length, LengthUnit>
IQuantity q; GetUnitInfo(IQuantity) UnitInfo
IQuantity<TUnit> q; GetUnitInfo(IQuantity) (fallback) UnitInfo

The IQuantity<TUnit> reference case loses its typed return — IQuantity<TUnit> does not satisfy the IQuantity<TQuantity, TUnit> constraint, so resolution falls back to the non-generic overload. That scenario is uncommon compared to the concretely-typed receiver case, which is now strictly better than before (returns UnitInfo<TQuantity, TUnit> instead of UnitInfo<TUnit>).

Follow-up

PR #1658 stacks on top of this branch: adds static abstract Info on IQuantityOfType<T> / IQuantity<TSelf, TUnitType>, obsoletes the instance QuantityInfo member, and provides matching GetQuantityInfo() extensions. Out of scope for this PR.

🤖 Generated with Claude Code

…all TFMs

Reverts the default interface members added in #1649 and instead exposes
GetUnitInfo() / GetUnitInfo<TUnit>() as extension methods on all target
frameworks (previously gated behind #if !NET).

Rationale (per @lipchev review feedback on #1649):
- DIM bloats the IQuantity contract with members that just delegate to
  QuantityInfo[UnitKey]; aim is a leaner interface.
- A custom IQuantity implementor overriding UnitInfo would silently
  diverge from internal lookups (UnitConverter, UnitAbbreviationsCache
  etc.) which always go through QuantityInfo, not the new property.
- Calling the DIM through the IQuantity interface on a struct quantity
  causes interface dispatch and boxing per call.

The extension method has none of these issues and gives the same
ergonomic call-site (quantity.GetUnitInfo()) on every TFM.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented May 1, 2026

Code Review — PR #1657: Revert IQuantity.UnitInfo DIM, expose GetUnitInfo() extension on all TFMs

Overview

This PR reverts the default interface member IQuantity.UnitInfo added in #1649 and instead makes GetUnitInfo() / GetUnitInfo<TUnit>() extension methods available on all TFMs by dropping the #if !NET guard. Net result: −26 lines, simpler cross-TFM API.


Breaking Changes

IQuantity.UnitInfo and IQuantity<TUnitType>.UnitInfo are removed. Any caller on an IQuantity reference needs to migrate:

// before
UnitInfo info = quantity.UnitInfo;

// after
UnitInfo info = quantity.GetUnitInfo();

Since UnitInfo was only added in #1649 (very recently), the blast radius should be small in practice. Still worth a clear note in the changelog/release notes.


Rationale — Agreed

The three reasons given in the PR description are all solid:

  1. Lean interface — the DIM was just QuantityInfo[UnitKey], no new capability.
  2. Semantic trap — a custom IQuantity implementor overriding the DIM would silently diverge from UnitConverter, UnitAbbreviationsCache, etc., which all use QuantityInfo directly. No compile-time signal of the inconsistency.
  3. Boxing cost on structs — interface dispatch boxes the receiver; since concrete quantities don't (and can't) override a DIM, every Mass.Zero.UnitInfo-via-IQuantity would box.

Code Quality

  • Clean diff. 8 additions / 34 deletions; the remaining #if NET guards in IQuantity.cs (for QuantityInfo IQuantity.QuantityInfo, Enum IQuantity.Unit) are untouched and correct.
  • Stale XML docs removed. The <remarks> saying "On .NET 5+ targets, available as a DIM instead" in QuantityExtensions.cs are correctly stripped.
  • No orphaned usages. Verified via grep — no remaining quantity.UnitInfo calls on IQuantity references in the codebase.
  • Test renames follow the project's naming convention (GetUnitInfo_ReturnsUnitInfoForQuantityUnit, etc.) and the assertions are identical to before.

Minor Notes

  • The Get prefix on GetUnitInfo() is slightly inconsistent with other extensions like As() and ToUnit(), but it makes sense here: it signals a dictionary lookup rather than a trivial property access, and mirrors GetHashCode() / GetType() precedent for non-trivial accessors.
  • The follow-up described in the PR (static abstract Info on IQuantityOfType<TQuantity>) is a good direction; having it scoped out explicitly keeps this PR focused.

Summary

This is a well-reasoned, minimal revert. The rationale is convincing and the implementation is clean. LGTM once the breaking change is noted in the release notes.

@lipchev
Copy link
Copy Markdown
Collaborator

lipchev commented May 1, 2026

@angularsen This is surely better than having them on the interface, but here are my thoughts:

  • the generic version uses IQuantity<TUnit> resulting in boxing when used with something like Mass.Zero (while still producing TUnit-number of concrete implementations)
  • I haven't tested it, but I'm pretty sure both extension members would show up in the Mass.Zero. auto-completion list
  • We could potentially ship these (and others like them) in separate nuggets, like the number extensions- using something like 'CSharp14' as a suffix where they could live as regular extension properties (note, I'm 99% certain, those can still target netstandard2.0)

Bottom line- I don't mind the addition, but if I were to ever need to use one of these properties in my own projects- I'd probably write my own extension block.

@angularsen
Copy link
Copy Markdown
Owner Author

Autocomplete shows both in Rider, but I think that is fine:

image

It favors the typed one:
image

Regarding boxing, this only applies when calling Mass.Zero.GetUnitInfo(), and this is not something I expect anyone to do in a hot path so I really think that's fine.

At the end of the day, these are convenience additions that also improve on discoverability - but at least now it's reusing the same global configuration of custom quantities instead of whatever the instance implementation decides to deliver. I don't think this is worth shoving into a separate nuget, since it adds very little bloat.

@lipchev
Copy link
Copy Markdown
Collaborator

lipchev commented May 2, 2026

I'm fine either way, but still can't see any good uses for having a convenience method for the UnitInfo<TUnit> - if it were the UnitInfo<TQuantity, TUnit> I guess I could see it being used as an instance factory or something, but this just feel like another set of breaking changes to avoid in the future.

Replaces the IQuantity<TUnit> -> UnitInfo<TUnit> overload with
IQuantity<TQuantity, TUnit> -> UnitInfo<TQuantity, TUnit>, so the
typed extension returns the most specific UnitInfo shape when the
caller has a concretely-typed quantity (e.g. Length).

Behavior:
- Length q; q.GetUnitInfo() -> UnitInfo<Length, LengthUnit>
- IQuantity q; q.GetUnitInfo() -> UnitInfo (non-generic fallback)
- IQuantity<TUnit> q; q.GetUnitInfo() -> UnitInfo (non-generic fallback)

The IQuantity<TUnit> reference case loses its typed UnitInfo<TUnit>
return, but that scenario is uncommon compared to the concretely-typed
receiver case which now returns the richer UnitInfo<TQuantity, TUnit>.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented May 2, 2026

Code Review

Overview

Reverts the IQuantity.UnitInfo and IQuantity<TUnitType>.UnitInfo default interface members from #1649, and exposes GetUnitInfo() as extension methods available on all TFMs (removing the #if !NET guard).

Design

The rationale is well explained: DIMs on a struct-based type add boxing cost for every call, and a custom IQuantity implementor overriding UnitInfo would silently diverge from what UnitConverter/UnitAbbreviationsCache/QuantityFormatter actually use internally. The extension-method form avoids both problems.

The overload resolution table in the PR description is accurate and helpful:

  • Length q -> infers GetUnitInfo<Length, LengthUnit> -> returns UnitInfo<Length, LengthUnit> (richer)
  • IQuantity q -> GetUnitInfo(IQuantity) -> returns UnitInfo (non-generic fallback)
  • IQuantity<TUnit> q -> falls back to non-generic overload (loses typed return vs previous UnitInfo<TUnit>)

The loss of typed return for IQuantity<TUnit> callers is an acceptable trade-off given how rarely that specific constraint appears compared to concretely-typed receivers.

Breaking Changes

Removes IQuantity.UnitInfo and IQuantity<TUnitType>.UnitInfo DIMs. Anyone who added these properties to a custom IQuantity implementation will get an "interface member not found" error on upgrade if they only override the removed DIM. Low risk since the DIMs were just introduced in #1649.

Tests

Renaming UnitInfo_* tests to GetUnitInfo_* is clean. The new test for GetUnitInfo<TQuantity, TUnit> returning the fully-typed UnitInfo<Length, LengthUnit> is a good regression guard for the overload resolution.

Summary

Clean revert + improvement. No concerns.

@angularsen
Copy link
Copy Markdown
Owner Author

I changed it to return UnitInfo<TQuantity, TUnit> instead of UnitInfo<TUnit>.

@angularsen angularsen enabled auto-merge (squash) May 2, 2026 16:02
@angularsen angularsen disabled auto-merge May 2, 2026 17:38
@angularsen angularsen merged commit ad445ef into master May 2, 2026
2 of 4 checks passed
@angularsen angularsen deleted the claude/revert-iquantity-unitinfo-dim branch May 2, 2026 17:38
angularsen added a commit that referenced this pull request May 2, 2026
PR #1657's CI failed at restore time with:

  error NU1901: Warning As Error: Package 'NuGet.Packaging' 7.0.1 has
    a known low severity vulnerability
  error NU1901: Warning As Error: Package 'NuGet.Protocol' 7.0.1 has
    a known low severity vulnerability

These are pulled in transitively by build tooling (CodeGen) and
cannot be upgraded without breaking other constraints.

Two changes:
- Add NU1901 (low) and NU1902 (moderate) to WarningsNotAsErrors in
  Directory.Build.props so they remain visible as warnings but no
  longer fail the build via TreatWarningsAsErrors. High (NU1903) and
  critical (NU1904) advisories still fail the build.
- CodeGen.csproj had its own WarningsNotAsErrors that overrode (not
  appended to) the one in Directory.Build.props. Prefix it with
  $(WarningsNotAsErrors); so the project inherits the NU codes (and
  the obsolete codes) while keeping its nullability suppressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
angularsen added a commit that referenced this pull request May 2, 2026
PR #1657's CI failed at restore time with:

  error NU1901: Warning As Error: Package 'NuGet.Packaging' 7.0.1 has
    a known low severity vulnerability
  error NU1901: Warning As Error: Package 'NuGet.Protocol' 7.0.1 has
    a known low severity vulnerability

These are pulled in transitively by build tooling (CodeGen) and
cannot be upgraded without breaking other constraints.

Two changes:
- Add NU1901 (low) and NU1902 (moderate) to WarningsNotAsErrors in
  Directory.Build.props so they remain visible as warnings but no
  longer fail the build via TreatWarningsAsErrors. High (NU1903) and
  critical (NU1904) advisories still fail the build.
- CodeGen.csproj had its own WarningsNotAsErrors that overrode (not
  appended to) the one in Directory.Build.props. Prefix it with
  $(WarningsNotAsErrors); so the project inherits the NU codes (and
  the obsolete codes) while keeping its nullability suppressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
angularsen added a commit that referenced this pull request May 2, 2026
PR #1657's CI failed at restore time with:

  error NU1901: Warning As Error: Package 'NuGet.Packaging' 7.0.1 has
    a known low severity vulnerability
  error NU1901: Warning As Error: Package 'NuGet.Protocol' 7.0.1 has
    a known low severity vulnerability

These are pulled in transitively by build tooling (CodeGen) and
cannot be upgraded without breaking other constraints.

Two changes:
- Add NU1901 (low) and NU1902 (moderate) to WarningsNotAsErrors in
  Directory.Build.props so they remain visible as warnings but no
  longer fail the build via TreatWarningsAsErrors. High (NU1903) and
  critical (NU1904) advisories still fail the build.
- CodeGen.csproj had its own WarningsNotAsErrors that overrode (not
  appended to) the one in Directory.Build.props. Prefix it with
  $(WarningsNotAsErrors); so the project inherits the NU codes (and
  the obsolete codes) while keeping its nullability suppressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
angularsen added a commit that referenced this pull request May 2, 2026
## Summary

Stops the build from failing on low/moderate NuGet audit advisories
while keeping them visible as warnings.

## Why

PR #1657's CI failed at restore time with:

```
error NU1901: Warning As Error: Package 'NuGet.Packaging' 7.0.1 has a known low severity vulnerability
error NU1901: Warning As Error: Package 'NuGet.Protocol' 7.0.1 has a known low severity vulnerability
```

These are pulled in transitively by build tooling (CodeGen) and cannot
be upgraded without breaking other constraints.

## Changes

- Add `NU1901` (low) and `NU1902` (moderate) to `<WarningsNotAsErrors>`
in `Directory.Build.props`. They remain visible as warnings but no
longer fail the build via `TreatWarningsAsErrors`. `NU1903` (high) and
`NU1904` (critical) still fail the build.
- `CodeGen.csproj` had its own `<WarningsNotAsErrors>` that overrode
(not appended to) the one in `Directory.Build.props`. Prefix it with
`$(WarningsNotAsErrors);` so the project inherits the NU codes (and the
obsolete codes) while keeping its nullability suppressions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
angularsen added a commit that referenced this pull request May 2, 2026
## Summary

Bumps centrally-managed NuGet package versions in
[Directory.Packages.props](Directory.Packages.props) to the latest
stable release.

| Package | From | To |
|---------|------|----|
| Microsoft.NET.Test.Sdk | 18.0.1 | 18.5.1 |
| NuGet.Protocol | 7.0.1 | 7.3.1 |
| Serilog | 4.3.0 | 4.3.1 |

`NuGet.Protocol` 7.3.1 clears the low-severity NU1901 advisories that
triggered the original CI failure on #1657. As a result this PR makes
#1660 unnecessary as a build-unblocker, though the comment-and-policy
clean-up there still has independent value.

`System.CommandLine.DragonFruit` is left at the existing
`0.4.0-alpha.22272.1`; no newer stable version is published to the
configured sources.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
angularsen added a commit that referenced this pull request May 2, 2026
Per discussion on #1657, this introduces a static abstract `Info` member
on the IQuantityOfType<TQuantity> and IQuantity<TSelf,TUnitType>
interfaces under #if NET, marks the existing instance QuantityInfo
property as [Obsolete] on .NET 5+, and adds GetQuantityInfo() extension
methods on QuantityExtensions so callers have a single discoverable API
that works on every TFM.

Why
- The instance QuantityInfo property invariably returns a per-type
  static value. Exposing it as an instance member implies it can vary
  per instance, which it cannot, and incurs interface dispatch (boxing
  on structs) for every call.
- The static abstract member lets generic algorithms reach the info
  with `TSelf.Info` directly, no boxing, no virtual call.
- The extension method pair (`GetQuantityInfo()` /
  `GetQuantityInfo<TUnit>()`) is the discoverable replacement for
  callers that only have an `IQuantity` reference. It looks the
  quantity up via `UnitsNetSetup.Default.Quantities`.
- Keeping the instance property obsolete (warning) instead of removing
  it preserves source compatibility for existing callers and the
  netstandard2.0 contract. We can promote to error / remove once
  netstandard2.0 is dropped.

Implementation notes
- Generated quantities already expose `public static QuantityInfo<TSelf,
  TUnitType> Info { get; }`, which directly satisfies the typed static
  abstract. The non-generic `IQuantityOfType<TSelf>.Info` is satisfied
  by a default static implementation in IQuantity<TSelf,TUnitType>:
  `static QuantityInfo IQuantityOfType<TSelf>.Info => TSelf.Info;`.
  No codegen change required.
- The IQuantity bridge `QuantityInfo IQuantity.QuantityInfo =>
  QuantityInfo;` chain inside the interfaces uses #pragma to suppress
  the obsolete warning on the bridge itself.
- Internal callers in UnitsNet were migrated to either `TSelf.Info` /
  `TSelf.From` (where the generic constraint allows) or
  `quantity.GetQuantityInfo()` (where it doesn't). Callers that must
  keep working for custom quantities not registered in
  `UnitsNetSetup.Default` (JsonNet serialization, debugger proxy,
  QuantityTypeConverter) keep using the instance member with a
  `#pragma warning disable CS0618` and a comment explaining why.
- HowMuch test custom quantity changed `public static readonly` field
  to `public static QuantityInfo Info { get; }` property to satisfy the
  static abstract.
- Tests added for round-trip equivalence between `Mass.Info`,
  `mass.GetQuantityInfo()`, `TQuantity.Info` (via static abstract on
  IQuantityOfType<T>), and `TSelf.Info` (via static abstract on
  IQuantity<TSelf,TUnit>).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants