🐛 Fix incorrect caching of XML documentation comments with/without expandIncludes by sharwell · Pull Request #36021 · dotnet/roslyn

sharwell · 2019-05-29T14:10:01Z

No description provided.

jcouv · 2019-05-29T17:16:27Z

Please provide some context for reviewers.
What is the scenario impacted? Can it be validated with test?

AlekseyTs · 2019-05-29T17:23:38Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceEventSymbol.cs

        private SymbolCompletionState _state;
        private CustomAttributesBag<CSharpAttributeData> _lazyCustomAttributesBag;
        private string _lazyDocComment;
+        private string _lazyExpandedDocComment;


private string _lazyExpandedDocComment; [](start = 8, length = 39)

Is there a big advantage to cache this? Is it common to request this information? #Closed

I'm not sure how valuable the cache is. I have observed documentation comment evaluation as a substantial contributor to some poor performance scenarios, but that's not a direct indicator of the helpfulness of the cache (in theory or as implemented). This commit was part of an experimental branch where I updated Quick Info to expand included documentation as part of the presentation process (which I believe is the correct approach), only to find that in some cases the results were not getting expanded.

I'm not sure how valuable the cache is.

We might want to take approach used for PE Symbols. It looks like implementations of GetDocumentationCommentXml for them cache result of the first request and recalculate if subsequent requests use different parameters.

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

We might want to take approach used for PE Symbols. It looks like implementations of GetDocumentationCommentXml for them cache result of the first request and recalculate if subsequent requests use different parameters.

The PE symbol implementation looks at the culture info, which (IMO) seems less likely to change during a session. It ignores the expandIncludes parameter.

With that said, either approach works for me. I am working on tests which cover the API for source symbols. Currently I've implemented the VB side the same way I implemented the C# side in this pull request. If you prefer it the other way, I can switch them all.

AlekseyTs · 2019-05-29T17:28:18Z

Does VB have the same problem? #Closed

AlekseyTs · 2019-05-29T17:29:09Z

Done with review pass (iteration 1). Please add tests covering the affected code paths. #Closed

sharwell · 2019-05-29T17:51:28Z

Does VB have the same problem?

Yes, but I have not updated those cases. Definitely an oversight on my part.

Please add tests covering the affected code paths.

Can you help identify a good place for these tests? I have some tests in another branch that cover these paths, but the tests are for a somewhat unrelated feature in the IDE layers. It would be better to have more localized tests in the compiler layer.

AlekseyTs · 2019-05-29T18:45:15Z

Can you help identify a good place for these tests?

It looks like we have a bunch of tests for GetDocumentationCommentXml API in src\Compilers\CSharp\Test\Symbol\DocumentationComments folder.

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

sharwell · 2019-05-30T00:09:33Z

It looks like we have a bunch of tests for GetDocumentationCommentXml API in src\Compilers\CSharp\Test\Symbol\DocumentationComments folder.

Great! I'm adding a new IncludeTests.cs in this folder. To start with I'm only covering the behavior I changed and not actually writing tests for the full <include> functionality.

sharwell · 2019-05-30T15:17:29Z

@AlekseyTs VB implementation and tests are now added

sharwell · 2019-05-30T19:17:19Z

@AlekseyTs It looks like the code might not be correctly using the culture provided to GetDocumentationCommentXml. I don't know what the preferred approach is for resolving the remaining test failure.

AlekseyTs · 2019-05-30T20:05:37Z

@sharwell

It looks like the code might not be correctly using the culture provided to GetDocumentationCommentXml. I don't know what the preferred approach is for resolving the remaining test failure.

Could you provide more details about the failures? #Closed

sharwell · 2019-05-30T20:10:30Z

The Spanish leg is failing because it prints a localized error message even when GetDocumentationCommentXml is called with EnsureEnglishUICulture.PreferredOrNull.

AlekseyTs · 2019-05-30T20:18:43Z

The Spanish leg is failing because it prints a localized error message even when GetDocumentationCommentXml is called with EnsureEnglishUICulture.PreferredOrNull.

This is likely due to the fact that error messages aren't really a part of a doc comment, they are coming from somewhere else and are localized based on the current culture. Consider adding using (new EnsureEnglishUICulture()) in affected tests. There are examples of doing this in our tests. #Closed

…ndIncludes

AlekseyTs · 2019-05-31T19:08:00Z

@sharwell In the future, please, consider to not rewrite commits in the PRs that are being reviewed. This prevents people from reviewing incrementally and often can lead to losing mapping of comments to relevant positions.

sharwell · 2019-05-31T19:14:53Z

@AlekseyTs I made sure to not rebase so the incremental changes were preserved by the force-push events in the timeline without bringing in changes from other branches.

https://github.com/dotnet/roslyn/compare/d3c3aeaac82304aa59848ac37606c32104565c84..551e31cb96d1d35056271613bb36efb3feb4a776

https://github.com/dotnet/roslyn/compare/551e31cb96d1d35056271613bb36efb3feb4a776..a3ef8ae87557887f09c0647da35fafaa41c4b39b

src/Compilers/VisualBasic/Portable/DocumentationComments/SourceDocumentationCommentUtils.vb

AlekseyTs · 2019-05-31T19:25:53Z

@sharwell

I made sure to not rebase so the incremental changes were preserved by the force-push events in the timeline without bringing in changes from other branches.

There is only one commit in this PR now, there is no way for me to separate what changes I already looked at and what changes you've made after that. Significant portion of work done during previous review pass cannot be reused or built upon because I have to review everything from scratch. So, please, consider avoiding rewriting commits, push additional commits and squash/rebase at the end once review process is completed.

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceEventSymbol.vb

AlekseyTs

LGTM (iteration 1)

sharwell · 2019-05-31T19:39:08Z

there is no way for me to separate what changes I already looked at and what changes you've made after that

I submitted https://github.com/maintainers/early-access-feedback/issues/255 requesting improved clarity of the relation between the pull request at the time of review and the most recent iteration of the pull request.

AlekseyTs · 2019-05-31T20:13:20Z

I submitted https://github.com/maintainers/early-access-feedback/issues/255 requesting improved clarity of the relation between the pull request at the time of review and the most recent iteration of the pull request.

For some reason the link doesn't work for me, so I cannot comment on the request.

AlekseyTs · 2019-05-31T20:14:28Z

Note, however, that often we are not using GitHub UI for review.

src/Compilers/CSharp/Portable/Symbols/Source/SourceFieldSymbol.cs

jcouv

LGTM Thanks (iteration 1)

* Use ref locals to simplify code flow * Fix VB code layout

jcouv · 2019-06-10T17:12:01Z

Feel free to merge when CI is green. Thanks

sharwell requested a review from a team as a code owner May 29, 2019 14:10

sharwell added the Area-Compilers label May 29, 2019

sharwell changed the title ~~Fix incorrect caching of XML documentation comments with/without expandIncludes~~ 🐛 Fix incorrect caching of XML documentation comments with/without expandIncludes May 29, 2019

jcouv added this to the 16.2.P3 milestone May 29, 2019

AlekseyTs reviewed May 29, 2019

View reviewed changes

sharwell mentioned this pull request May 29, 2019

Implement Quick Info support for 'inheritdoc' #36028

Merged

sharwell force-pushed the comment-caching branch 2 times, most recently from 3eafe02 to 551e31c Compare May 30, 2019 15:17

Fix incorrect caching of XML documentation comments with/without expa…

a3ef8ae

…ndIncludes

sharwell force-pushed the comment-caching branch from 551e31c to a3ef8ae Compare May 30, 2019 21:05

sharwell closed this May 31, 2019

sharwell reopened this May 31, 2019

AlekseyTs reviewed May 31, 2019

View reviewed changes

src/Compilers/VisualBasic/Portable/DocumentationComments/SourceDocumentationCommentUtils.vb Outdated Show resolved Hide resolved

AlekseyTs reviewed May 31, 2019

View reviewed changes

src/Compilers/VisualBasic/Portable/Symbols/Source/SourceEventSymbol.vb Show resolved Hide resolved

AlekseyTs approved these changes May 31, 2019

View reviewed changes

jcouv reviewed Jun 6, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceFieldSymbol.cs Show resolved Hide resolved

jcouv self-assigned this Jun 6, 2019

jcouv approved these changes Jun 6, 2019

View reviewed changes

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

Clean up code style

0bdbfb0

* Use ref locals to simplify code flow * Fix VB code layout

sharwell merged commit f8c58ae into dotnet:master Jun 10, 2019

sharwell deleted the comment-caching branch June 10, 2019 23:25

Conversation

sharwell commented May 29, 2019

Uh oh!

jcouv commented May 29, 2019

Uh oh!

AlekseyTs May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell May 29, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 29, 2019

Choose a reason for hiding this comment

Uh oh!

sharwell May 30, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented May 29, 2019

Uh oh!

AlekseyTs commented May 29, 2019

Uh oh!

sharwell commented May 30, 2019

Uh oh!

sharwell commented May 30, 2019

Uh oh!

sharwell commented May 30, 2019

Uh oh!

AlekseyTs commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented May 30, 2019

Uh oh!

AlekseyTs commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented May 31, 2019

Uh oh!

Uh oh!

AlekseyTs commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented May 31, 2019

Uh oh!

AlekseyTs commented May 31, 2019

Uh oh!

AlekseyTs commented May 31, 2019

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs May 29, 2019 •

edited

Loading

AlekseyTs commented May 29, 2019 •

edited

Loading

AlekseyTs commented May 29, 2019 •

edited

Loading

AlekseyTs commented May 30, 2019 •

edited

Loading

AlekseyTs commented May 30, 2019 •

edited

Loading

AlekseyTs commented May 31, 2019 •

edited

Loading

AlekseyTs commented May 31, 2019 •

edited

Loading