Override completion must not include inaccessible parameter attributes by ivanbasov · Pull Request #36163 · dotnet/roslyn

ivanbasov · 2019-06-04T22:19:53Z

CyrusNajmabadi · 2019-06-04T23:54:31Z

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs

                    modifiers: modifiers,
+                    // There is no need for an override to have same parameters attributes as the original method.
+                    // Just clear attributes when generating an override.
+                    parameters: overriddenMethod.Parameters.SelectAsArray(p => p.WithAttributes(ImmutableArray<AttributeData>.Empty)),


what about overridden properties in VB? they can have parameters too :) #Resolved

genlu · 2019-06-05T18:05:45Z

Would missing the attribute in overriding method cause anything unexpected if the overridden method is abstract? #Resolved

jasonmalinowski

Code seems OK, but worried that this is the right thing to do or if we need a less aggressive hammer.

jasonmalinowski · 2019-06-05T18:13:12Z

src/Workspaces/Core/Portable/Shared/Extensions/SyntaxGeneratorExtensions.cs

                    method: overriddenMethod,
                    accessibility: overriddenMethod.ComputeResultantAccessibility(newContainingType),
                    modifiers: modifiers,
+                    // There is no need for an override to have same parameters attributes as the original method.


I worry this is a bit strong of a statement that there's no need -- how do we know there aren't some reflection-based systems out there that are depending on this?

Which attributes triggered this original question? #Resolved

We already do not copy attributes of methods and attributes of properties. By mistake (my opinion), we copy attributes of method parameters and property parameters. We just should stop doing this.

In reply to: 290869693 [](ancestors = 290869693)

CyrusNajmabadi · 2019-06-05T18:15:06Z

Code seems OK, but worried that this is the right thing to do or if we need a less aggressive hammer.

FWIW, i think this is totally ok. I recall us doing this in many other scenarios (maybe implement interface) and it really hasn't been a problem. This feels more like a missed case, versus something that is important to preserve behavior on. #Resolved

src/Workspaces/Core/Portable/CodeGeneration/CodeGenerationSymbolFactory.cs

ivanbasov · 2019-06-07T01:10:56Z

As far as I know, member signatures do not depend on attributes. In some cases, one may want some attributes to be copied or added. However, we do not need to guarantee this.

If you have an example of the opposite, we definitely should consider it.

In reply to: 499193025 [](ancestors = 499193025)

ivanbasov · 2019-06-07T01:25:04Z

support of property parameters added
feedback responded

@CyrusNajmabadi , @genlu , @jasonmalinowski please review

ivanbasov · 2019-06-07T19:56:44Z

There is a test failing: TestPropertyReturnTypeAttributes
The test was added by @CyrusNajmabadi to fix http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/546779
saying if we copy parameter attributes for properties, we also need to copy return type attributes for properties in case scenario:

Imports System.Runtime.InteropServices
 
Interface I
    Property P(<MarshalAs(UnmanagedType.I4)> x As Integer) As <MarshalAs(UnmanagedType.I4)> Integer
End Interface
 
Class C
    Implements I ' Implement
End Class

This is the concern that @genlu and @jasonmalinowski mentioned. The question is: do we need to copy attributes for any purpose? I'd say - no. See ideas in comments above. @CyrusNajmabadi , @jasonmalinowski , @genlu - your thoughts?

jasonmalinowski · 2019-06-10T22:44:55Z

The COM marshalling case does seem worrisome: I would be most surprised if the CLR walked to the base type to find the marshalling attributes before knowing how to marshal things. And in that case, those attributes are 100% critical to the correct functioning of the method.

More generally, the only way this is safe to blanket apply to all methods is if we know that every framework using reflection or metadata walking is going to look to the base type. That's a huge proof I don't think we can possibly make. And I can go and write a code that undoes your proof even if you did enumerate every package on NuGet.

Looking at the other bug, if the concern is NullableAttribute then we should close this PR and dupe it against one of the work items for nullable, since we need to address that attribute holistically and we're still having conversations about the best way to do it. In any case, this PR isn't the right way to fix that. If some other attribute is causing us to look at this then let's discuss that attribute -- but otherwise I'd absolutely close this PR and by design the original bug, making sure we have a tracking bug for NullableAttribute as appropriate.

ivanbasov · 2019-06-12T01:32:04Z

Synced with @jasonmalinowski offline. Agreed to keep accessible attributes only. Decided to add a method to determine attributes to be kept vs attributes to be dropped. @jasonmalinowski will extend it for other purposes later.

@jasonmalinowski , @genlu , @CyrusNajmabadi please review.

Override completion must not include parameter attributes

e400a93

ivanbasov added Area-IDE IDE-IntelliSense Completion, Signature Help, Quick Info IDE-CodeStyle Built-in analyzers, fixes, and refactorings labels Jun 4, 2019

ivanbasov added this to the 16.2.P3 milestone Jun 4, 2019

ivanbasov requested review from a team, Cosifne, CyrusNajmabadi, JoeRobich, dpoeschl and genlu June 4, 2019 22:19

CyrusNajmabadi reviewed Jun 4, 2019

View reviewed changes

sharwell removed the IDE-CodeStyle Built-in analyzers, fixes, and refactorings label Jun 5, 2019

jasonmalinowski reviewed Jun 5, 2019

View reviewed changes

genlu reviewed Jun 5, 2019

View reviewed changes

src/Workspaces/Core/Portable/CodeGeneration/CodeGenerationSymbolFactory.cs Show resolved Hide resolved

support of property parameters

b756bef

jinujoseph modified the milestones: 16.2.P3, 16.2 Jun 9, 2019

code review feedback

7ea8482

jasonmalinowski approved these changes Jun 12, 2019

View reviewed changes

ivanbasov merged commit e9a9904 into dotnet:master Jun 12, 2019

ivanbasov deleted the override branch June 12, 2019 20:55

jasonmalinowski changed the title ~~Override completion must not include parameter attributes~~ Override completion must not include inaccessible parameter attributes Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override completion must not include inaccessible parameter attributes#36163

Override completion must not include inaccessible parameter attributes#36163
ivanbasov merged 3 commits intodotnet:masterfrom
ivanbasov:override

ivanbasov commented Jun 4, 2019

Uh oh!

CyrusNajmabadi Jun 4, 2019 •

edited by ivanbasov

Loading

Uh oh!

genlu commented Jun 5, 2019 •

edited by ivanbasov

Loading

Uh oh!

jasonmalinowski left a comment

Uh oh!

jasonmalinowski Jun 5, 2019 •

edited by ivanbasov

Loading

Uh oh!

ivanbasov Jun 7, 2019

Uh oh!

CyrusNajmabadi commented Jun 5, 2019 •

edited by ivanbasov

Loading

Uh oh!

Uh oh!

ivanbasov commented Jun 7, 2019

Uh oh!

ivanbasov commented Jun 7, 2019

Uh oh!

ivanbasov commented Jun 7, 2019

Uh oh!

jasonmalinowski commented Jun 10, 2019 •

edited

Loading

Uh oh!

ivanbasov commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

ivanbasov commented Jun 4, 2019

Uh oh!

CyrusNajmabadi Jun 4, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu commented Jun 5, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Jun 5, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jun 5, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ivanbasov commented Jun 7, 2019

Uh oh!

ivanbasov commented Jun 7, 2019

Uh oh!

ivanbasov commented Jun 7, 2019

Uh oh!

jasonmalinowski commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanbasov commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CyrusNajmabadi Jun 4, 2019 •

edited by ivanbasov

Loading

genlu commented Jun 5, 2019 •

edited by ivanbasov

Loading

jasonmalinowski Jun 5, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi commented Jun 5, 2019 •

edited by ivanbasov

Loading

jasonmalinowski commented Jun 10, 2019 •

edited

Loading