-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
Tagging subscribers to this area: @dotnet/area-meta Issue Detailsnull
|
bf0c2ed
to
716b83c
Compare
More violations to fix.. |
@marek-safar do you plan to continue? |
Might be easiest to just merge some fixes at first and not attempt to get clean and enable the rule all in one PR |
f8a44e4
to
cefb77d
Compare
...libraries/System.Diagnostics.PerformanceCounter/src/System/Diagnostics/PerformanceCounter.cs
Outdated
Show resolved
Hide resolved
...erformanceCounter/src/System/Diagnostics/PerformanceData/CounterSetInstanceCounterDataSet.cs
Outdated
Show resolved
Hide resolved
....DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/LdapPal.Linux.cs
Outdated
Show resolved
Hide resolved
@@ -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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 useArray.Empty<T>()
if it's available, or elsenew T[0]
. - When emitting code for
$"Hello, {name}"
, it'll useDefaultInterpolatedStringBuilder
if available, otherwise it'll usestring.Format
. - When emitting code for await, if the thing you're awaiting has
UnsafeOnCompleted
it'll use it, otherwise it'll useOnCompleted
.
...
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
} | ||
_readState = ReadState.Interactive; | ||
return true; | ||
throw NotImplemented.ByDesign; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
...me.InteropServices/gen/LibraryImportGenerator/Analyzers/CustomMarshallerAttributeAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.Mono.cs
Show resolved
Hide resolved
There was a problem hiding this 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 ;-)
Failures are timeouts |
@marek-safar After this change I can no longer build locally.
|
Looks like unlucky timing with #77777 |
Looking... |
Follow up on #67527 which was closed meantime