Skip to content

Implement generate method and constructor with top-level nullability tracking wrapper#36049

Merged
jasonmalinowski merged 11 commits intodotnet:masterfrom
jasonmalinowski:implement-generate-method-with-nullable-wrapper
Jun 5, 2019
Merged

Implement generate method and constructor with top-level nullability tracking wrapper#36049
jasonmalinowski merged 11 commits intodotnet:masterfrom
jasonmalinowski:implement-generate-method-with-nullable-wrapper

Conversation

@jasonmalinowski
Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski commented May 29, 2019

This is an implementation of generate method with nullability, which tries to use an experimental approach of creating our own implementation of ITypeSymbol that carries along top-level nullability. This means we can avoid having to change all our internal helpers away from passing around ITypeSymbol and incrementally update the support for the IDE.

Commit-at-a-time review suggested, since that'll split apart the wrappers themselves along with the actual "work" needed to fix the feature.

@jasonmalinowski jasonmalinowski self-assigned this May 29, 2019
@jasonmalinowski jasonmalinowski changed the title Implement generate method with nullable wrapper Implement generate method with top-level nullability tracking wrapper May 30, 2019
@jasonmalinowski jasonmalinowski force-pushed the implement-generate-method-with-nullable-wrapper branch from 7fba867 to ec4e35f Compare May 30, 2019 01:32
CyrusNajmabadi
CyrusNajmabadi reviewed May 30, 2019
View reviewed changes
...table/GenerateMember/GenerateParameterizedMember/CSharpGenerateParameterizedMemberService.cs Outdated
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi May 30, 2019

Choose a reason for hiding this comment

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

should CreateTypeParameterSymbol just return a symbol that has the nullability?

CyrusNajmabadi
CyrusNajmabadi reviewed May 30, 2019
View reviewed changes
src/Workspaces/Core/Portable/Utilities/NullableHelpers/AllNullabilityIgnoringSymbolComparer.cs Outdated
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi May 30, 2019

Choose a reason for hiding this comment

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

so, this def doesn't seem like it would work for anything but types. This may likely matter. for example, with implement-interface, we may compare a method like Foo(string?) to Foo(string) and want them to be equal in terms of how interface-implementation sees things.

CyrusNajmabadi
CyrusNajmabadi reviewed May 30, 2019
View reviewed changes
src/Workspaces/Core/Portable/Utilities/NullableHelpers/ArrayTypeSymbolWithNullableAnnotation.cs Outdated
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi May 30, 2019

Choose a reason for hiding this comment

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

so, we're following compiler semantics here around equals. namely that it doesn't consider nullability?

Copy link
Copy Markdown
Member Author

@jasonmalinowski jasonmalinowski May 30, 2019

Choose a reason for hiding this comment

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

Well, right now this Equals implements the same semantics that the compiler currently implements in that it actually does look at nullability. Once we do the pass to use explicit comparers everywhere then this will be somewhat moot.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented May 30, 2019

Overall 👍 on the effort. If anything, this PR just makes me think the end outcome needs to be "the compiler absorbs this" :). Right now, it feels like you'll have to basically audit/update nearly every use of compiler APIs, effectively causing a ton of rewriting.

@jasonmalinowski jasonmalinowski force-pushed the implement-generate-method-with-nullable-wrapper branch from ec4e35f to ce49932 Compare May 30, 2019 16:28
@sharwell

This comment has been minimized.

Sign in to view
@jasonmalinowski

This comment has been minimized.

Sign in to view
@jasonmalinowski jasonmalinowski changed the title Implement generate method with top-level nullability tracking wrapper Implement generate method and constructor with top-level nullability tracking wrapper May 31, 2019
@jasonmalinowski jasonmalinowski marked this pull request as ready for review May 31, 2019 17:54
@jasonmalinowski jasonmalinowski requested a review from a team as a code owner May 31, 2019 17:54
@jasonmalinowski jasonmalinowski force-pushed the implement-generate-method-with-nullable-wrapper branch from 5bed8ea to 147946c Compare May 31, 2019 19:55
@jasonmalinowski jasonmalinowski requested a review from ryzngard May 31, 2019 20:00
ryzngard
ryzngard approved these changes May 31, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Overall everything looks good to me, with the expected caveats of some things are hacky and make us all sad. Would like to see WithoutNullability rename happen, but I won't block on it.

@gafter gafter added the Area-IDE label Jun 3, 2019
@jasonmalinowski
Add a set of wrapper ITypeSymbols that carry nullability along
077d536 
Currently, the APIs for nullable reference types take a model that
instead of the top level nullability of a named type being on the
INamedTypeSymbol, it's carried along as a separate piece of information.
This causes a lot of churn for code that is used to passing along
ITypeSymbols today; one way to ease this is simply create a set of
wrapper types which carry it along for us but still implement
ITypeSymbol.
@jasonmalinowski
Implement support in Generate Method and Constructor for nullability
0c305dd 
This enables basic support for applying top-level nullability to
parameters and the return types when generating methods.

Fixes dotnet#30319
@jasonmalinowski
Pass a CancellationToken when we have one
8eab0e0
@jasonmalinowski
Add some more UnwrapNullabilitySymbol() calls
dc54a43 
This catches places we're calling Construct() or
ClassifyConversion().
@jasonmalinowski
Fix up target type completion to ignore nullability
53ed1cf
@jasonmalinowski
Ignore nullabilities when comparing parameter types when generating e…
b3251e5 
…xceptions
@jasonmalinowski
Add more UnwrapNullabilitySymbol calls
dffce7f
@jasonmalinowski
Add workaround to IMethodSymbolExtensions.RenameTypeParameters
0a90194 
This makes the extension method work again. The workaround is tracked by
dotnet#36093.
@jasonmalinowski
Add an optimization to avoid re-creating wrappers if the same
44ddbb8
@jasonmalinowski
Ensure that constructing a generic type doesn't loose nullability
ce9c278
@jasonmalinowski
Rename UnwrapNullabilitySymbol to WithoutNullability
68159bd
@jasonmalinowski jasonmalinowski force-pushed the implement-generate-method-with-nullable-wrapper branch from 147946c to 68159bd Compare June 4, 2019 19:21
@jasonmalinowski jasonmalinowski merged commit a666539 into dotnet:master Jun 5, 2019
@jasonmalinowski jasonmalinowski deleted the implement-generate-method-with-nullable-wrapper branch June 5, 2019 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@ryzngard ryzngard ryzngard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jasonmalinowski jasonmalinowski

Labels

Area-IDE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jasonmalinowski @CyrusNajmabadi @sharwell @ryzngard @gafter