Update Roslyn snapshot when editorconfig is updated. for now, it only… by heejaechang · Pull Request #26221 · dotnet/roslyn

heejaechang · 2018-04-18T01:23:07Z

… updates when editorconfig is saved.

Customer scenario

User updates editorconfig and save the file but the change doesn't get picked up by other files opened in VS until files are closed and reopened.

Bugs this fixes

#15003

Workarounds, if any

modify the file or close and reopen the file.

Risk

Low.

Performance impact

Low

Is this a regression from a previous update?

No

Root cause analysis

We consider options as a part of snapshot. but we didn't update snapshots when editorconfig options are changed. for now, we support it for opened files, but for full experience, it require editorconfig work from compiler.

How was the bug found?

Dogfooding, Feedbacks

… updates when editorconfig is saved.

heejaechang · 2018-04-18T01:23:37Z

@jasonmalinowski is this approach look okay for now?

jasonmalinowski · 2018-04-18T01:27:05Z

@heejaechang Will look in detail tomorrow, but gist seems OK.

jasonmalinowski · 2018-04-18T01:27:12Z

(wrong button)

CyrusNajmabadi · 2018-04-18T19:22:11Z

src/Workspaces/Core/Portable/Workspace/Solution/Solution.cs

+        internal Solution WithOptionChanged()
+        {
+            // just create new snapshot
+            return new Solution(_state);


this needs more explanation. for example, why this has any effect at all. From just looking it woudl seem that this does nothing since we're just makinga new solution with exactly the same state as the current solution we have.

there are bunch of comments what it is doing. we made option as part of solution snapshot. but when option is changed, we don't create new snapshot. so it does that now.

@CyrusNajmabadi Basically, Document holds onto a snapshot of the file's options, so that way that's immutable. When the options change, we aren't (currently) creating a new snapshot so values would be stale. This is at least working around that problem before we integrate .editorconfig more cleanly into workspaces, which is currently underway.

@jasonmalinowski What i meant was: we need to document that on the call. :)

jinujoseph · 2018-04-18T22:12:31Z

approved to merge via link for 15.7.Preview5

jinujoseph · 2018-04-18T22:12:55Z

one more review pending

sharwell

A couple questions to answer 😄

sharwell · 2018-04-18T22:14:25Z

src/EditorFeatures/Core.Wpf/Options/EditorConfigDocumentOptionsProvider.cs

+                        {
+                            var context = t.Result;
+
+                            context.CodingConventionsChangedAsync -= OnCodingConventionsChangedAsync;


❓ Does this matter if we are disposing of the object anyway?

I don't know how dispose is implemented in external component. but I won't rely on their implementation but just be good citizen and unsubscribe events handler I subscribed.

if it was our component that we can control implementation, then, might be different story. but not for external one.

This is an editor's component, but please be a good citizen still :)
I wonder if you need to unsubscribe before scheduling a task though, what if the .editorconfig file has changed in between lines 52 and 57 and OnCodingConventionsChangedAsync gets called?

sharwell · 2018-04-18T22:15:44Z

src/EditorFeatures/Core.Wpf/Options/EditorConfigDocumentOptionsProvider.cs

                defaultValue: EmptyCodingConventionContext.Instance);
        }
+
+        private Task OnCodingConventionsChangedAsync(object sender, CodingConventionsChangedEventArgs arg)


❓ @olegtk Is this event ever invoked more often than one invocation for one change of .editorconfig on disk?

if you are asking whether changing 1 editconfig file that affects multiple files will cause multiple events to be fired. then I tested it, and they do fire events for each file that got affected by the edit config file.

there should be 1 event per file change (as raised by the VS file watcher service), and it's fired on each affected context instance.

olegtk

looks good from coding conventions side

olegtk · 2018-04-18T23:34:29Z

src/EditorFeatures/Core.Wpf/Options/EditorConfigDocumentOptionsProvider.cs

+                        {
+                            var context = t.Result;
+
+                            context.CodingConventionsChangedAsync -= OnCodingConventionsChangedAsync;


This is an editor's component, but please be a good citizen still :)
I wonder if you need to unsubscribe before scheduling a task though, what if the .editorconfig file has changed in between lines 52 and 57 and OnCodingConventionsChangedAsync gets called?

olegtk · 2018-04-18T23:46:26Z

src/EditorFeatures/Core.Wpf/Options/EditorConfigDocumentOptionsProvider.cs

                defaultValue: EmptyCodingConventionContext.Instance);
        }
+
+        private Task OnCodingConventionsChangedAsync(object sender, CodingConventionsChangedEventArgs arg)


there should be 1 event per file change (as raised by the VS file watcher service), and it's fired on each affected context instance.

heejaechang · 2018-04-19T00:53:30Z

alright. thank you every one. addressed some PR feedbacks.

tagging @jinujoseph

first, this PR only update snapshot if open file is affected by editor config file change. closed file, it is not easy to do. that will be automatically supported once we have full editor config support from compiler.

second, I did event aggregation. this is good for perf but now we have chance that if users use options within 50ms since last editconfig change events, they will not see the option changes.

heejaechang · 2018-04-19T00:53:46Z

opened tracking issue
#26250

Questions were answered

heejaechang · 2018-04-19T05:37:36Z

@dotnet/roslyn-infrastructure do I need to keep rerun integration test until it pass or is it okay if one of test (release) passed?

heejaechang · 2018-04-19T06:35:21Z

retest windows_debug_vs-integration_prtest please

… it only… (dotnet#26221)" This reverts commit f56d67a. We are seeing deadlocks where our code tries to fetch the options for a file, and that code runs on the background thread. The background thread is trying to hop back to the foreground thread to apply file watchers for .editorconfig files.

Update Roslyn snapshot when editorconfig is updated. for now, it only…

fcb8589

… updates when editorconfig is saved.

heejaechang requested a review from a team as a code owner April 18, 2018 01:23

heejaechang changed the base branch from master to dev15.7.x April 18, 2018 01:23

jasonmalinowski closed this Apr 18, 2018

jasonmalinowski reopened this Apr 18, 2018

jinujoseph added the Needs Shiproom Approval label Apr 18, 2018

CyrusNajmabadi reviewed Apr 18, 2018

View reviewed changes

jasonmalinowski approved these changes Apr 18, 2018

View reviewed changes

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Apr 18, 2018

sharwell previously requested changes Apr 18, 2018

View reviewed changes

added more comments

aa65b01

olegtk approved these changes Apr 18, 2018

View reviewed changes

heejaechang mentioned this pull request Apr 19, 2018

Remove workaround added in https://github.com/dotnet/roslyn/pull/26221 once compiler editorconfig work is done #26250

Closed

addressed PR feedbacks

e9c6460

heejaechang merged commit f56d67a into dotnet:dev15.7.x Apr 19, 2018

jcouv added the Area-IDE label Apr 19, 2018

jinujoseph mentioned this pull request Apr 19, 2018

Newly added .editorconfig only applied after reloading files #15003

Closed

heejaechang mentioned this pull request Apr 25, 2018

3 changes around editorconfig #26386

Closed

Conversation

heejaechang commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

heejaechang commented Apr 18, 2018

Uh oh!

jasonmalinowski commented Apr 18, 2018

Uh oh!

jasonmalinowski commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang Apr 18, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinujoseph commented Apr 18, 2018

Uh oh!

jinujoseph commented Apr 18, 2018

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olegtk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang commented Apr 19, 2018

Uh oh!

heejaechang commented Apr 19, 2018

Uh oh!

heejaechang commented Apr 19, 2018

Uh oh!

heejaechang commented Apr 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

heejaechang commented Apr 18, 2018 •

edited

Loading

heejaechang Apr 18, 2018 •

edited

Loading