DateOnly.AddX methods should be marked [Pure]

hughesjs commented 2 years ago

Description

The methods in System.DateOnly such as .AddYears(int) are pure in that they create a new instance of DateOnly with the modified values.

However, they haven't been decorated with the [Pure] attribute which means that IDEs (and users by extension) might not be aware of this and might be expecting it to mutate the variable itself. IIRC, most IDEs will generate a warning if you don't capture the value of a [Pure] method call.

Tbh, this probably ought to be a compiler warning as well.

Reproduction Steps

var x = DateOnly.MinValue;
// These lines should produce a warning about not capturing the value of a pure method
x.AddYears(1);
x.AddDays(1);
x.AddMonths(1);

Expected behavior

At the very least, IDEs that are aware of the [Pure] attribute (I believe as a minimum VS and Rider both are) should display a squiggly about not capturing the value of a pure method.

Ideally, the compiler should also generate a warning.

Actual behavior

No warnings are created as the methods aren't declared as pure.

Regression?

No

Known Workarounds

No response

Configuration

Version: .NET 6.0 OS: Mac OS 11.6.1 Arch: x64 Config Specific: No

Other information

No response

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

KalleOlaviNiemitalo commented 2 years ago

Some previous requests to add [Pure] elsewhere were rejected. See https://github.com/dotnet/runtime/issues/24659, https://github.com/dotnet/runtime/issues/33414#issuecomment-597043981, https://github.com/dotnet/runtime/issues/34098#issuecomment-604439020.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

Issue Details

### Description The methods in `System.DateOnly` such as `.AddYears(int)` are pure in that they create a new instance of `DateOnly` with the modified values. However, they haven't been decorated with the `[Pure]` attribute which means that IDEs (and users by extension) might not be aware of this and might be expecting it to mutate the variable itself. IIRC, most IDEs will generate a warning if you don't capture the value of a `[Pure]` method call. Tbh, this probably ought to be a compiler warning as well. ### Reproduction Steps ```cs var x = DateOnly.MinValue; // These lines should produce a warning about not capturing the value of a pure method x.AddYears(1); x.AddDays(1); x.AddMonths(1); ``` ### Expected behavior At the very least, IDEs that are aware of the `[Pure]` attribute (I believe as a minimum VS and Rider both are) should display a squiggly about not capturing the value of a pure method. Ideally, the compiler should also generate a warning. ### Actual behavior No warnings are created as the methods aren't declared as pure. ### Regression? No ### Known Workarounds _No response_ ### Configuration Version: .NET 6.0 OS: Mac OS 11.6.1 Arch: x64 Config Specific: No ### Other information _No response_

Author:	hughesjs
Assignees:	-
Labels:	`area-System.Runtime`, `untriaged`
Milestone:	-

tannergooding commented 2 years ago

IDEs and tooling that only consider the Pure attribute are "outdated" in my opinion.

The Pure attribute has never been actually enforced and never had "significant" usage throughout the BCL or other key foundations of the ecosystem.

Modern C#/.NET have newer concepts such as readonly struct and readonly members that do at least get language validation and enforcement (ignoring unsafe code) and the "purity" of something like DateTime.AddX is implied by it being an instance member of a readonly struct. The same goes for methods like Vector4.CopyTo, where Vector4 isn't readonly but the CopyTo method is and that indicates that the method cannot mutate instance state (and this is validated by the language for safe code).

The concept from "pure" that is missing is for static/instance methods mutating static state, but due to back-compat among other things I would expect that the existing Pure attribute couldn't be reused or enforced. It would end up being some new language enforced attribute if that was ever introduced.

stephentoub commented 2 years ago

Yes, as I outlined in https://github.com/dotnet/runtime/issues/34098#issuecomment-604439020, I'd be happy to see us introduce a [DoNotIgnoreResult] attribute and associated analyzer, but that's distinct from [Pure]. I don't want to further propagate the use of [Pure].

tannergooding commented 2 years ago

Modern .NET likewise provides two built-in analyzers covering "you forgot to use the value":

These cover the scenario of "you called a method that returns a value, but aren't consuming the value anywhere".

dotnet / runtime