Support having a file be both a document and additional file. #20558

Pilchie · 2017-06-30T00:01:26Z

Ask Mode

Customer scenario
If a single filepath is included in both the Compile and the AdditionalFiles item groups, the project system would fail to add the second of those two, and throw, leaving things in a bad state. In the legacy project system, that seemed to be caught and ignored, and likely you just didn't have the AdditionalFile in your compilations. In CPS a dialog would be displayed, and the file would end up in Miscellaneous files (however, this can happen during a transient state in CPS due to it's async nature). In Lightweight solution load, VS would crash.

Bugs this fixes: Fixes #19968
Workarounds, if any: Not have the item in both Compile and AdditionalFiles. Hard, because they may not realize they do, or it may be a transient case in CPS.
Risk: Medium - plumbs through new ids for additional items, etc.
Performance impact: Low - most significant change is ref counting instead of mere entry in a dictionary.
Is this a regression from a previous update?: No, this behavior has been since additional files were added.
Root cause analysis:: We never expected an item to be in both groups.
How was the bug found?: Customers reported files ending up in "Miscellaneous files" in CPS after changing the build action.

…ditional documents

Pilchie · 2017-06-30T00:02:29Z

I recommend reading commit-by-commmit.

Note that we can debate about whether the public API changes are warranted or not.

Pilchie · 2017-06-30T00:11:22Z

Note: @davkean with this changed, if I renamed such a file in solution explorer in a CPS project I got tons of ugly exceptions from the project system...

DustinCampbell

I'd be up for debating the public API change. I get that the parameter was meaningless, but removing it seems an unnecessary binary breaking change.

DustinCampbell · 2017-06-30T00:05:16Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/AbstractProject.cs

+            var moniker = document.Key.Moniker;
+            if (_documentMonikers.TryGetValue(moniker, out var value))
+            {
+                _documentMonikers[moniker] = (value.document, value.refCount++);


I assume the post-increment happens before the assignment?

Ugh. I'll separate it just to not have to think about this.

DustinCampbell · 2017-06-30T00:07:16Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/AbstractProject.cs

+        private void RemoveMoniker(IVisualStudioHostDocument document)
+        {
+            var moniker = document.Key.Moniker;
+            var value = _documentMonikers[moniker];


Any reason to protect against the moniker not being in the Dictionary? If it's not, this will throw.

Good catch, the code previously just had _documentMonikers.Remove(), which wouldn't throw. I'll guard.

Pilchie · 2017-06-30T00:24:11Z

Re: public API - going to take a look at removing all the internal bits, but leaving the public entry points with comments saying that they are ignored. I sort of agree that it's not worth breaking.

Turns out we could end up with the same IVisualStudioHostDocument for both, if they are both added. Fixes dotnet#19968.

This is a breaking API change. Need to decide if we want to take it or not.

Pilchie · 2017-06-30T02:05:48Z

@DustinCampbell care to take another look? Also - do you think I should squash the last two commits?

davkean · 2017-06-30T12:12:29Z

@Pilchie What kind of ugly errors?

davkean · 2017-06-30T12:14:15Z

Oh you mean if you renamed a file that was both a compile and additional file you got ugly errors? ie Not a regression introduced by this change?

Pilchie · 2017-06-30T15:48:10Z

@davkean Right - with this change, if you F2 and rename a file in solution explorer that is both source and additional files there are a bunch of exceptions from the project system about the tree not being consistent.

Pilchie · 2017-06-30T17:31:15Z

retest windows_debug_vs-integration_prtest please

Pilchie · 2017-06-30T17:31:24Z

Hit the vsix installer error.

Pilchie · 2017-06-30T17:31:34Z

Tagging @MattGertz for approval here.

Pilchie · 2017-06-30T17:46:39Z

retest windows_debug_vs-integration_prtest please

Pilchie · 2017-06-30T19:47:28Z

retest windows_debug_vs-integration_prtest please

jasonmalinowski

The overall approach seems sound. Some detail questions about the deprecated parameter (the comment doesn't make sense to me) and other questions.

jasonmalinowski · 2017-06-30T20:06:35Z

src/EditorFeatures/Core/Shared/Preview/PreviewWorkspace.cs

+        /// Puts the specified additional document into the open state.
+        /// </summary>
+        /// <param name="documentId">The <see cref="DocumentId"/> to open.</param>
+        /// <param name="activate">Ignored - not necessary for additional documents.</param>


Isn't the statement for the param here more "this doesn't matter in the preview workspace?"

jasonmalinowski · 2017-06-30T20:07:45Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/AbstractProject.cs

            }

            UninitializeAdditionalDocument(document);
        }

+        private void AddMoniker(IVisualStudioHostDocument document)


"AddToMonikerToDocumentMap"?

jasonmalinowski · 2017-06-30T20:08:06Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/AbstractProject.cs

+            if (_documentMonikers.TryGetValue(moniker, out var value))
+            {
+                value.refCount++;
+                _documentMonikers[moniker] = (value.document, value.refCount);


I must admit: using tuples here seems really terrifying and really likely to make a mistake....

Can't the right hand side just be "value" since you already have the tuple in hand though?

jasonmalinowski · 2017-06-30T20:11:40Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/AbstractProject.cs

+                }
+                else
+                {
+                    _documentMonikers[moniker] = (value.document, value.refCount);


Can't the right hand side just be "value" since you already have the tuple in hand though?

Pilchie · 2017-06-30T21:46:41Z

retest windows_debug_vs-integration_prtest please

Pilchie · 2017-06-30T22:34:56Z

A bunch of tests failed in https://ci.dot.net/job/dotnet_roslyn/job/dev15.3.x/job/windows_debug_vs-integration_prtest/15/ with remoting exceptions.

Pilchie · 2017-06-30T22:35:10Z

retest windows_debug_vs-integration_prtest please

…teSourceAndAdditionalFiles-DistinctIds" This reverts commit 6f5b2d4, reversing changes made to 66362df.

Revert #20558

…-DuplicateSourceAndAdditionalFiles-DistinctIds"" This reverts commit d03cde8.

Use different host documents and document ids for source files and ad…

5af1d1e

…ditional documents

dnfclas added the cla-already-signed label Jun 30, 2017

Pilchie requested review from jasonmalinowski, davkean, mattwar, mavasani and a team June 30, 2017 00:01

DustinCampbell reviewed Jun 30, 2017

View reviewed changes

Pilchie mentioned this pull request Jun 30, 2017

New Quick Info API #20554

Merged

5 tasks

Pilchie added 6 commits June 29, 2017 19:04

Refcount document monikers for source and additional files

ec766cd

Turns out we could end up with the same IVisualStudioHostDocument for both, if they are both added. Fixes dotnet#19968.

Style updates

b904ab8

Add DebuggerDisplay for DocumentKey

d6cdf10

Don't set current context for AdditionalDocuments

b4a6e6b

Remove currentContext parameter for Additional Documents

8de15ee

This is a breaking API change. Need to decide if we want to take it or not.

Revert public API changes

818b0a7

Pilchie force-pushed the Fix19968-DuplicateSourceAndAdditionalFiles-DistinctIds branch from 81bc054 to 818b0a7 Compare June 30, 2017 02:04

MattGertz added the Pending director approval label Jun 30, 2017

Pilchie mentioned this pull request Jun 30, 2017

Having an item that is both a source file and an additional item breaks DPL and CPS #19968

Closed

MattGertz added Approved to merge and removed Pending director approval labels Jun 30, 2017

jasonmalinowski approved these changes Jun 30, 2017

View reviewed changes

Pilchie merged commit 6f5b2d4 into dotnet:dev15.3.x Jun 30, 2017

Pilchie deleted the Fix19968-DuplicateSourceAndAdditionalFiles-DistinctIds branch June 30, 2017 23:09

rchande pushed a commit to rchande/roslyn that referenced this pull request Jul 6, 2017

Revert "Merge pull request dotnet#20558 from Pilchie/Fix19968-Duplica…

d03cde8

…teSourceAndAdditionalFiles-DistinctIds" This reverts commit 6f5b2d4, reversing changes made to 66362df.

rchande mentioned this pull request Jul 6, 2017

Revert https://github.com/dotnet/roslyn/pull/20558 #20687

Merged

rchande pushed a commit that referenced this pull request Jul 6, 2017

Merge pull request #20687 from rchande/revert20558

ee1637c

Revert #20558

Pilchie added a commit to Pilchie/roslyn that referenced this pull request Jul 24, 2017

Revert "Revert "Merge pull request dotnet#20558 from Pilchie/Fix19968…

0670e78

…-DuplicateSourceAndAdditionalFiles-DistinctIds"" This reverts commit d03cde8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support having a file be both a document and additional file. #20558

Support having a file be both a document and additional file. #20558

Pilchie commented Jun 30, 2017 •

edited

Loading

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

DustinCampbell left a comment

DustinCampbell Jun 30, 2017

Pilchie Jun 30, 2017

DustinCampbell Jun 30, 2017

Pilchie Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

davkean commented Jun 30, 2017

davkean commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

jasonmalinowski left a comment

jasonmalinowski Jun 30, 2017

jasonmalinowski Jun 30, 2017

jasonmalinowski Jun 30, 2017

jasonmalinowski Jun 30, 2017

jasonmalinowski Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Support having a file be both a document and additional file. #20558

Support having a file be both a document and additional file. #20558

Conversation

Pilchie commented Jun 30, 2017 • edited Loading

Ask Mode

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

davkean commented Jun 30, 2017

davkean commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017

Pilchie commented Jun 30, 2017 •

edited

Loading