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

Allow xml docs to still be created when 'emit metadata only' is on. #57667

Merged
merged 17 commits into from
Jan 21, 2022

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Nov 10, 2021

EmitMetadataOnly is a flag that exists for IDE scenarios to allow it to emit dlls cheaply that give hte top-level surface area of a particular compilation. This can then be used in cross-language scenarios to allow things like intellisense/QI/and-lots-more to work in a live fashion. We call these cross-language metadata-only dlls: 'skeleton assemblies'.

The IDE would like to improve our story around skeleton assemblies by being able to cache them to our persistence store so that later launched sessions of VS can reuse them without recomputing them.

This is generally not difficult for us to do except for one small snag. Specifically, one thing we support today with skeletons is being able to expose xml doc comments through the skeleton. This works by having us actually both emit the metadta-only-dll, but also creating a DocumentationProvider that wraps the original language compilation. So, for example, if VB references C#, it will get a dll for the C# metadata, but it will get a doc-provider that forwards along to the C# compilation to answer questions.

If we want to persist the dll (and still have xml-doc-comments work across languages) then we need to persist the xml doc info as well. This PR updates the compiler to support that. Previously, the compiler would completely skip xml doc generation if EmitMetadataOnly was set. With this PR, the compiler will emit xml doc comments as well if the appropriate stream is provided.

Relates to ref assemblies: #18612

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler ptal :)

@jcouv jcouv self-assigned this Nov 15, 2021
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 7)

{
public static void SayHello()
{
ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole.WriteLine(""hello"");
Copy link
Member

Choose a reason for hiding this comment

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

ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole

Accidental paste?

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 10) modulo typo in test

@jcouv
Copy link
Member

jcouv commented Jan 18, 2022

@dotnet/roslyn-compiler for second review. Thanks

@RikkiGibson RikkiGibson self-assigned this Jan 18, 2022
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM except the feedback above (just have to address before merge).

xmlDocBytes = xmlStream.ToArray();
}

// This should not fail the emit (as it's a warning).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please remove this comment here and the one below // Even though docs failed, ...

@jcouv jcouv marked this pull request as draft January 20, 2022 03:35
@jcouv
Copy link
Member

jcouv commented Jan 20, 2022

Feel free to merge/squash once the typo and last comment are addressed/considered. Thanks

{
public static void SayHello()
{
ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole.WriteLine(""hello"");
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
ConEmitMetadataOnly_XmlDocs_NoDocMode_Successsole.WriteLine(""hello"");
EmitMetadataOnly_XmlDocs_NoDocMode_Success.WriteLine(""hello"");

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right, aren't we trying to call Console.WriteLine here?

Copy link
Member Author

Choose a reason for hiding this comment

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

sigh...

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review January 20, 2022 20:10
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) January 20, 2022 20:10
@CyrusNajmabadi CyrusNajmabadi merged commit 5daad64 into dotnet:main Jan 21, 2022
@ghost ghost added this to the Next milestone Jan 21, 2022
333fred added a commit to 333fred/roslyn that referenced this pull request Jan 21, 2022
* upstream/main: (1035 commits)
  Add missing header
  Mark IVSTypeScriptFormattingServiceImplementation as optional, but require it in the constructor
  Fix Go To Base for a symbol with a single metadata location (dotnet#58965)
  [EnC] Store entire spans instead of line deltas (dotnet#58831)
  Delete CodeAnalysisRules.ruleset (dotnet#58968)
  Allow xml docs to still be created when 'emit metadata only' is on. (dotnet#57667)
  Fix ParseVBErrorOrWarning (dotnet#47833)
  Update parameter nullability to match implementation
  Ensure CSharpUseParameterNullCheckingDiagnosticAnalyzer works with nullable parameters
  Add tests for issues fixed by previous PR (dotnet#58764)
  Update src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs
  Disallow null checking discard parameters (dotnet#58952)
  Add extension method
  Escape type arguments
  Few fixes
  Update tests.
  Add Analyzers layer to CODEOWNERS
  Add formatting analyzer test for param nullchecking (dotnet#58936)
  Move reading HideAdvancedMembers option up (dotnet#58747)
  List patterns: Slice value is assumed to be never null (dotnet#57457)
  ...
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the metadataOnlyEmitXmlDocs branch December 8, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants