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

Teach GenApi to understand nullability #2679

Merged
merged 17 commits into from
Sep 27, 2019
Merged

Conversation

safern
Copy link
Member

@safern safern commented May 3, 2019

I marked the PR as WIP since I still want to implement a cache for the MetadataReader, use the document pointers used by the HostEnvironment to load the assemblies instead of using the locations and opening a file and creating a PeReader every time. (However this is ready to use in the meantime).

This teaches GenAPI to understand nullable attributes and emit ? correctly.

While doing this work I found that GenAPI didn't understand DynamicAttribute and didn't support nested ValueTuples so I fixed those as part of this work as well.

I needed to pull in a dependency into System.Reflection.Metadata in order to be able to get the custom attributes from GenericConstraintTable and InterfaceImplTable since CCI doesn't expose the attributes for those guys, making it imposible to annotate interfaces, generic constraints and explicit implementations correctly. The reasoning of pulling in System.Reflection.Metadata instead of modifying CCI was because we want to move out from using CCI sometime soon.

** Here you can find a gist of a sample input and output after these changes **

cc: @dotnet/nullablefc @danmosemsft

@safern safern requested a review from ericstj May 3, 2019 01:08
@terrajobst
Copy link
Member

Nice! The dependency on S.R.MD is unfortunate, but it is what it is.

Long term, I'd like us to move away from MS.Cci and move to S.R.MetadataLoadContext instead. However, this probably requires us to expose some these things in reflection as well.

@ericstj
Copy link
Member

ericstj commented May 3, 2019

Even better might be to use Roslyn if possible.

@safern safern closed this May 17, 2019
@safern safern reopened this May 17, 2019
@safern safern force-pushed the GenApiNullable branch 2 times, most recently from 26a5f32 to 49fac80 Compare May 29, 2019 00:54
@terrajobst
Copy link
Member

Would you mind rebasing this on top of master again? This way, I can get a coherent build of API Reviewer that can diff attributes and see nullable annotations.

@safern
Copy link
Member Author

safern commented Jul 3, 2019

Yes. I will rebase when I get back to it and update it to understand the new compact metadata.

@safern
Copy link
Member Author

safern commented Jul 19, 2019

Pending:

  • Add tests.
  • Add MetadataReader cache.

@safern
Copy link
Member Author

safern commented Jul 26, 2019

@ericstj I added the MetadataReader cache, and it improved the execution time by half a second so I think it is worth having it.

Also at the end I decided to not add it to the Host as I didn't want to make the CSDeclarationWriter dependent on the Host as it seems like they were intentionally decoupled, since the CSDeclarationWriter only needs an IEnumerable.

@safern safern changed the title [WIP] Teach GenApi to understand nullability Teach GenApi to understand nullability Sep 20, 2019
@safern
Copy link
Member Author

safern commented Sep 20, 2019

@ericstj this is also ready for review whenever you get a chance.

@safern
Copy link
Member Author

safern commented Sep 27, 2019

@dougbu this change will also need updated refs in the ingestion PR for asp.NET. I added an space after inlined attributes. Ran code check locally and that is the only impact of this change in aspnet extensions.

@safern safern merged commit 14abaee into dotnet:master Sep 27, 2019
@safern safern deleted the GenApiNullable branch September 27, 2019 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants