Add accessibility modifier on file creation by Youssef1313 · Pull Request #51935 · dotnet/roslyn

Youssef1313 · 2021-03-17T15:32:48Z

@sharwell Any concerns for the excessive usage of JoinableTaskFactory.Run?

sharwell · 2021-03-17T15:50:20Z

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.

Youssef1313 · 2021-03-17T16:13:35Z

Closing and reopening for a new build.

  Retrying 'FindPackagesByIdAsync' for source 'https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/microsoft.visualstudio.progression.common/index.json'.
  An error occurred while sending the request.
    The remote name could not be resolved: 'pkgs.dev.azure.com'
  Retrying 'FindPackagesByIdAsync' for source 'https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/microsoft.visualstudio.progression.interfaces/index.json'.
  An error occurred while sending the request.
    The remote name could not be resolved: 'pkgs.dev.azure.com'
  Retrying 'FindPackagesByIdAsync' for source 'https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/d1622942-d16f-48e5-bc83-96f4539e7601/nuget/v3/flat2/microsoft.visualstudio.progression.codeschema/index.json'.
  An error occurred while sending the request.
    The remote name could not be resolved: 'pkgs.dev.azure.com'
  Retrying 'FindPackagesByIdAsync' for source 'https://pkgs.dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_packaging/45bacae2-5efb-47c8-91e5-8ec20c22b4f8/nuget/v3/flat2/microsoft.visualstudio.progression.codeschema/index.json'.
  An error occurred while sending the request.
    The remote name could not be resolved: 'pkgs.dev.azure.com'

...Core/CodeFixes/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs

CyrusNajmabadi · 2021-03-17T17:34:14Z

...Core/CodeFixes/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs

-            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)


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.

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?

CyrusNajmabadi · 2021-03-17T17:34:50Z

src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs

            var documentOptions = ThreadHelper.JoinableTaskFactory.Run(() => addedDocument.GetOptionsAsync(cancellationToken));

+            // Add access modifier
+            var rootWithAccessModifier = ThreadHelper.JoinableTaskFactory.Run(() =>


ick. instead of this, can we move this all the way to the entrypoint and have it jsut call a clean async method?

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.

CyrusNajmabadi · 2021-03-17T18:07:06Z

so FormatDocumentCreatedFromTemplate should do the jtf run, and call into a pure async method FormatDocumentCreatedFromTemplateAsync.

Youssef1313 · 2021-07-28T18:05:08Z

@CyrusNajmabadi Thanks for taking this on!

sharwell · 2021-07-28T20:04:00Z

src/VisualStudio/Core/Def/Implementation/AbstractEditorFactory.cs

            // 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);


📝 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)

Add accessibility modifier on file creation

e4c90bb

Youssef1313 requested a review from a team as a code owner March 17, 2021 15:32

ghost added the Area-IDE label Mar 17, 2021

Update AbstractAddAccessibilityModifiersCodeFixProvider.cs

a358ab7

Youssef1313 closed this Mar 17, 2021

Youssef1313 reopened this Mar 17, 2021

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 17, 2021

CyrusNajmabadi reviewed Mar 17, 2021

View reviewed changes

...Core/CodeFixes/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 17, 2021

View reviewed changes

jinujoseph assigned CyrusNajmabadi Jul 28, 2021

CyrusNajmabadi added 2 commits July 28, 2021 10:12

Merge remote-tracking branch 'upstream/main' into cleanup-file-creation

02ed185

Move code.

1f97af2

CyrusNajmabadi enabled auto-merge July 28, 2021 17:44

fix

6a80471

Fix

9af41e4

sharwell reviewed Jul 28, 2021

View reviewed changes

sharwell approved these changes Jul 28, 2021

View reviewed changes

CyrusNajmabadi added 5 commits July 28, 2021 15:18

introduce service

9f796a6

Merge remote-tracking branch 'upstream/main' into cleanup-file-creation

c0a18e2

Merge branch 'main' into cleanup-file-creation

bf79730

move down

fda6aea

Use COnfigureAwait(true)

f8f5ab4

CyrusNajmabadi merged commit 011b722 into dotnet:main Jul 30, 2021

ghost added this to the Next milestone Jul 30, 2021

Youssef1313 deleted the cleanup-file-creation branch July 30, 2021 08:34

merriemcgaw mentioned this pull request Aug 18, 2021

Internal modifier added to partial classes breaks new project creation for WinForms, WebTools and anything using partial classes in their templates #55703

Closed

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add accessibility modifier on file creation#51935

Add accessibility modifier on file creation#51935
CyrusNajmabadi merged 11 commits intodotnet:mainfrom
Youssef1313:cleanup-file-creation

Youssef1313 commented Mar 17, 2021

Uh oh!

sharwell commented Mar 17, 2021

Uh oh!

Youssef1313 commented Mar 17, 2021

Uh oh!

Uh oh!

CyrusNajmabadi Mar 17, 2021

Uh oh!

Youssef1313 Mar 17, 2021

Uh oh!

CyrusNajmabadi Mar 17, 2021

Uh oh!

Youssef1313 Mar 17, 2021

Uh oh!

CyrusNajmabadi commented Mar 17, 2021

Uh oh!

Youssef1313 commented Jul 28, 2021

Uh oh!

sharwell Jul 28, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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));
	}

Conversation

Youssef1313 commented Mar 17, 2021

Uh oh!

sharwell commented Mar 17, 2021

Uh oh!

Youssef1313 commented Mar 17, 2021

Uh oh!

Uh oh!

CyrusNajmabadi Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Mar 17, 2021

Uh oh!

Youssef1313 commented Jul 28, 2021

Uh oh!

sharwell Jul 28, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sharwell Jul 28, 2021 •

edited

Loading