-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add accessibility modifier on file creation #51935
Add accessibility modifier on file creation #51935
Conversation
No performance concerns about JTF.Run on this particular path. After this is merged, the method could be refactored to make the containing method asynchronous instead, and use a single JTF.Run in the caller. |
Closing and reopening for a new build.
|
...Core/CodeFixes/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
Document document, ImmutableArray<Diagnostic> diagnostics, | ||
SyntaxEditor editor, CancellationToken cancellationToken) | ||
// Intended for use by AbstractEditorFactory to add accessibility when creating a new file. | ||
internal static async Task<SyntaxNode> GetTransformedSyntaxRootAsync(Document document, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in a codefix. we shoudl not be calling it from random code. INstead, make a helper somewhere and have both the feature and the editor call into that helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in a codefix. we shoudl not be calling it from random code. INstead, make a helper somewhere and have both the feature and the editor call into that helper.
I was following what's done with AbstractFileHeaderCodeFixProvider:
roslyn/src/Analyzers/Core/CodeFixes/FileHeaders/AbstractFileHeaderCodeFixProvider.cs
Line 64 in 6da1274
internal static async Task<SyntaxNode> GetTransformedSyntaxRootAsync(ISyntaxFacts syntaxFacts, AbstractFileHeaderHelper fileHeaderHelper, SyntaxTrivia newLineTrivia, Document document, CancellationToken cancellationToken) |
Any good place for it?
@@ -331,11 +332,20 @@ private void FormatDocumentCreatedFromTemplate(IVsHierarchy hierarchy, uint item | |||
Contract.ThrowIfNull(rootToFormat); | |||
var documentOptions = ThreadHelper.JoinableTaskFactory.Run(() => addedDocument.GetOptionsAsync(cancellationToken)); | |||
|
|||
// Add access modifier | |||
var rootWithAccessModifier = ThreadHelper.JoinableTaskFactory.Run(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ick. instead of this, can we move this all the way to the entrypoint and have it jsut call a clean async method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which entrypoint did you mean?
This is where the things are done currently:
roslyn/src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs
Lines 274 to 285 in a358ab7
int IVsEditorFactoryNotify.NotifyItemAdded(uint grfEFN, IVsHierarchy pHier, uint itemid, string pszMkDocument) | |
{ | |
// Is this being added from a template? | |
if (((__EFNFLAGS)grfEFN & __EFNFLAGS.EFN_ClonedFromTemplate) != 0) | |
{ | |
var waitIndicator = _componentModel.GetService<IWaitIndicator>(); | |
// TODO(cyrusn): Can this be cancellable? | |
waitIndicator.Wait( | |
"Intellisense", | |
allowCancel: false, | |
action: c => FormatDocumentCreatedFromTemplate(pHier, itemid, pszMkDocument, c.CancellationToken)); | |
} |
Probably not very trivial to make it a clean async method.
so FormatDocumentCreatedFromTemplate should do the jtf run, and call into a pure async method FormatDocumentCreatedFromTemplateAsync. |
@CyrusNajmabadi Thanks for taking this on! |
} | ||
|
||
// Organize using directives | ||
addedDocument = ThreadHelper.JoinableTaskFactory.Run(() => OrganizeUsingsCreatedFromTemplateAsync(addedDocument, cancellationToken)); | ||
rootToFormat = ThreadHelper.JoinableTaskFactory.Run(() => addedDocument.GetRequiredSyntaxRootAsync(cancellationToken).AsTask()); | ||
addedDocument = await OrganizeUsingsCreatedFromTemplateAsync(addedDocument, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 These can all be ConfigureAwait(true)
to improve efficiency (the main thread is the caller and available to process continuations without the potential latency of the thread pool)
Fixes #50797
@sharwell Any concerns for the excessive usage of
JoinableTaskFactory.Run
?