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

Enable IDE0060 (Remove unused parameter) analyzer #72667

Merged
merged 44 commits into from
Nov 8, 2022

Conversation

marek-safar
Copy link
Contributor

@marek-safar marek-safar commented Jul 22, 2022

Follow up on #67527 which was closed meantime

@ghost
Copy link

ghost commented Jul 22, 2022

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

Issue Details

null

Author: marek-safar
Assignees: -
Labels:

area-Meta

Milestone: -

@marek-safar marek-safar force-pushed the ide0060 branch 7 times, most recently from bf0c2ed to 716b83c Compare July 22, 2022 14:37
@danmoseley
Copy link
Member

More violations to fix..

@danmoseley
Copy link
Member

@marek-safar do you plan to continue?

@danmoseley danmoseley marked this pull request as draft September 6, 2022 02:13
@danmoseley
Copy link
Member

Might be easiest to just merge some fixes at first and not attempt to get clean and enable the rule all in one PR

@@ -406,7 +406,7 @@ internal static unsafe class PinningMarshaller
public static ref float GetPinnableReference(ColorMatrix managed) => ref (managed is null ? ref Unsafe.NullRef<float>() : ref managed.GetPinnableReference());

// All usages in our currently supported scenarios will always go through GetPinnableReference
public static float* ConvertToUnmanaged(ColorMatrix managed) => throw new UnreachableException();
public static float* ConvertToUnmanaged(ColorMatrix _) => throw new UnreachableException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkoritzinsky, if in such situations we statically know these methods will never be used, can we update the generator to be ok with them not existing, and then just delete them rather than having these throw new UnreachableException() dummy implementations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a lot of desire on the interop team to always require specific methods for particular shapes to reduce the concept count we'd have to teach users. cc: @AaronRobinsonMSFT since he felt the most strongly about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we update the generator to be ok with them not existing, and then just delete them

This breaks the interop contract and is something we are being very explicit about going forward. The intent here is to avoid at all costs magic from the source generator scenarios. We expect functions (A), (B), and (C), if that shape isn't there then it is a broken interop contract.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'm saying today it is:

  • "We expect functions (A), (B), and (C), if that shape isn't there then it is a broken interop contract.

but it could be:

  • "If (A) is available, we'll use A. If (B) is available, we'll use B. If (C) is available, we'll use C.".

The C# compiler does this in many places, for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C# compiler does this in many places, for example.

Can you share an example?

Copy link
Member

@stephentoub stephentoub Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share an example?

e.g.

  • If you foreach something where the enumerator implements IDisposable, the code will be emitted with a try/finally calling Dispose; if it doesn't, no try/finally, no Dispose call.
  • When calling M(params T[] args) with no args, it'll use Array.Empty<T>() if it's available, or else new T[0].
  • When emitting code for $"Hello, {name}", it'll use DefaultInterpolatedStringBuilder if available, otherwise it'll use string.Format.
  • When emitting code for await, if the thing you're awaiting has UnsafeOnCompleted it'll use it, otherwise it'll use OnCompleted.
    ...

}
_readState = ReadState.Interactive;
return true;
throw NotImplemented.ByDesign;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is deleting all of this valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was probably bad merge, reverted the change

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Other than my remaining pending comments, LGTM. I don't intend to review again ;-)

@marek-safar
Copy link
Contributor Author

Failures are timeouts

@marek-safar marek-safar merged commit 8918adf into dotnet:main Nov 8, 2022
@marek-safar marek-safar deleted the ide0060 branch November 8, 2022 13:05
@jakobbotsch
Copy link
Member

@marek-safar After this change I can no longer build locally. .\build.cmd clr+libs -rc checked -c release gives me tons of

C:\dev\dotnet\runtime\src\libraries\System.Private.CoreLib\src\System\Runtime\Intrinsics\Wasm\WasmBase.PlatformNotSupported.cs(36,49
): error IDE0060: Remove unused parameter 'value' [C:\dev\dotnet\runtime\src\coreclr\System.Private.CoreLib\System.Private.CoreLib.c
sproj]

@jakobbotsch
Copy link
Member

Looks like unlucky timing with #77777

@stephentoub
Copy link
Member

Looking...

ViktorHofer pushed a commit to dotnet/winforms that referenced this pull request Dec 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants