From 02b6fad8e0c2cdcae14b38cddc12e787ba538f61 Mon Sep 17 00:00:00 2001 From: Gen Lu Date: Thu, 27 May 2021 11:50:20 -0700 Subject: [PATCH 1/5] Fix optprof config drop path --- azure-pipelines-official.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure-pipelines-official.yml b/azure-pipelines-official.yml index c99f0904a424e..91df7e50c4dd0 100644 --- a/azure-pipelines-official.yml +++ b/azure-pipelines-official.yml @@ -211,7 +211,7 @@ stages: ARTIFACTSERVICES_DROP_PAT: $(_DevDivDropAccessToken) inputs: dropServiceURI: 'https://devdiv.artifacts.visualstudio.com' - buildNumber: 'ProfilingInputs/DevDiv/$(Build.Repository.Name)/$(SourceBranchName)/$(Build.BuildNumber)' + buildNumber: 'ProfilingInputs/$(System.TeamProject)/$(Build.Repository.Name)/$(SourceBranchName)/$(Build.BuildNumber)' sourcePath: '$(Build.SourcesDirectory)\artifacts\OptProf\$(BuildConfiguration)\Data' toLowerCase: false usePat: false From 5c9bcea570a9861120f7835c931bdfb939fa3505 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Fri, 21 May 2021 11:29:38 -0700 Subject: [PATCH 2/5] Null annotate IWorkspaceProjectContextFactory --- .../CPS/IWorkspaceProjectContextFactory.cs | 6 ++---- .../Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs | 10 ++++------ .../CPS/CPSProject_IWorkspaceProjectContext.cs | 2 +- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs index 240a399223658..35fcc226499a4 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs @@ -2,8 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -#nullable disable - using System; using System.Threading; using System.Threading.Tasks; @@ -17,7 +15,7 @@ internal interface IWorkspaceProjectContextFactory { /// [Obsolete("Use CreateProjectContextAsync instead")] - IWorkspaceProjectContext CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object hierarchy, string binOutputPath); + IWorkspaceProjectContext CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath); /// /// Creates and initializes a new Workspace project and returns a Project guid. /// Obsolete. The argument is ignored. /// Initial project binary output path. - Task CreateProjectContextAsync(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object hierarchy, string binOutputPath, CancellationToken cancellationToken); + Task CreateProjectContextAsync(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath, CancellationToken cancellationToken); } } diff --git a/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs b/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs index 35bc6f8fa07c5..4e4907f0c6e06 100644 --- a/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs +++ b/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs @@ -2,8 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -#nullable disable - using System; using System.ComponentModel.Composition; using System.Threading; @@ -43,7 +41,7 @@ public CPSProjectFactory( _serviceProvider = (Shell.IAsyncServiceProvider)serviceProvider; } - IWorkspaceProjectContext IWorkspaceProjectContextFactory.CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object hierarchy, string binOutputPath) + IWorkspaceProjectContext IWorkspaceProjectContextFactory.CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath) { return _threadingContext.JoinableTaskFactory.Run(() => this.CreateProjectContextAsync(languageName, projectUniqueName, projectFilePath, projectGuid, hierarchy, binOutputPath, CancellationToken.None)); @@ -52,10 +50,10 @@ IWorkspaceProjectContext IWorkspaceProjectContextFactory.CreateProjectContext(st public async Task CreateProjectContextAsync( string languageName, string projectUniqueName, - string projectFilePath, + string? projectFilePath, Guid projectGuid, - object hierarchy, - string binOutputPath, + object? hierarchy, + string? binOutputPath, CancellationToken cancellationToken) { await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); diff --git a/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs b/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs index 945de21385027..2baadb03afb48 100644 --- a/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs +++ b/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs @@ -61,7 +61,7 @@ public bool LastDesignTimeBuildSucceeded set => _visualStudioProject.HasAllInformation = value; } - public CPSProject(VisualStudioProject visualStudioProject, VisualStudioWorkspaceImpl visualStudioWorkspace, IProjectCodeModelFactory projectCodeModelFactory, Guid projectGuid, string binOutputPath) + public CPSProject(VisualStudioProject visualStudioProject, VisualStudioWorkspaceImpl visualStudioWorkspace, IProjectCodeModelFactory projectCodeModelFactory, Guid projectGuid, string? binOutputPath) { _visualStudioProject = visualStudioProject; _visualStudioWorkspace = visualStudioWorkspace; From bb7f6155742d511600e097fde8b4f482ed9488b6 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Fri, 21 May 2021 11:41:49 -0700 Subject: [PATCH 3/5] Add a property to pass through the initial assembly name Right now we don't get the command line string for options until the full design time build has completed; however the project system can still give us the evaluated string which is likely to be close enough. This allows features that want to get symbol names for source to have a better chance of having something that they can use and look up in caches or in the cloud. --- .../CSharp/Test/ProjectSystemShim/CSharpHelpers.cs | 2 ++ .../CPS/IWorkspaceProjectContextFactory.cs | 14 +++++++++++++- .../Impl/ProjectSystem/CPS/CPSProjectFactory.cs | 10 +++++++++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/VisualStudio/CSharp/Test/ProjectSystemShim/CSharpHelpers.cs b/src/VisualStudio/CSharp/Test/ProjectSystemShim/CSharpHelpers.cs index 41bc80f793c64..6bfdd9c096465 100644 --- a/src/VisualStudio/CSharp/Test/ProjectSystemShim/CSharpHelpers.cs +++ b/src/VisualStudio/CSharp/Test/ProjectSystemShim/CSharpHelpers.cs @@ -86,6 +86,7 @@ public static async Task CreateCSharpCPSProjectAsync(TestEnvironment projectGuid, hierarchy, binOutputPath, + assemblyName: null, CancellationToken.None); cpsProject.SetOptions(ImmutableArray.Create(commandLineArguments)); @@ -105,6 +106,7 @@ public static async Task CreateNonCompilableProjectAsync(TestEnviron Guid.NewGuid(), hierarchy, binOutputPath: null, + assemblyName: null, CancellationToken.None); } diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs index 35fcc226499a4..df0c894e5967f 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs @@ -17,6 +17,10 @@ internal interface IWorkspaceProjectContextFactory [Obsolete("Use CreateProjectContextAsync instead")] IWorkspaceProjectContext CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath); + /// + [Obsolete("Use CreateProjectContextAsync instead")] + IWorkspaceProjectContext CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath, string? assemblyName); + /// /// Creates and initializes a new Workspace project and returns a to lazily initialize the properties and items for the @@ -29,6 +33,14 @@ internal interface IWorkspaceProjectContextFactory /// Project guid. /// Obsolete. The argument is ignored. /// Initial project binary output path. - Task CreateProjectContextAsync(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath, CancellationToken cancellationToken); + Task CreateProjectContextAsync( + string languageName, + string projectUniqueName, + string projectFilePath, + Guid projectGuid, + object? hierarchy, + string? binOutputPath, + string? assemblyName, + CancellationToken cancellationToken); } } diff --git a/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs b/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs index 4e4907f0c6e06..302171e13e329 100644 --- a/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs +++ b/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs @@ -44,7 +44,13 @@ public CPSProjectFactory( IWorkspaceProjectContext IWorkspaceProjectContextFactory.CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath) { return _threadingContext.JoinableTaskFactory.Run(() => - this.CreateProjectContextAsync(languageName, projectUniqueName, projectFilePath, projectGuid, hierarchy, binOutputPath, CancellationToken.None)); + this.CreateProjectContextAsync(languageName, projectUniqueName, projectFilePath, projectGuid, hierarchy, binOutputPath, assemblyName: null, CancellationToken.None)); + } + + IWorkspaceProjectContext IWorkspaceProjectContextFactory.CreateProjectContext(string languageName, string projectUniqueName, string projectFilePath, Guid projectGuid, object? hierarchy, string? binOutputPath, string? assemblyName) + { + return _threadingContext.JoinableTaskFactory.Run(() => + this.CreateProjectContextAsync(languageName, projectUniqueName, projectFilePath, projectGuid, hierarchy, binOutputPath, assemblyName, CancellationToken.None)); } public async Task CreateProjectContextAsync( @@ -54,12 +60,14 @@ public async Task CreateProjectContextAsync( Guid projectGuid, object? hierarchy, string? binOutputPath, + string? assemblyName, CancellationToken cancellationToken) { await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); var creationInfo = new VisualStudioProjectCreationInfo { + AssemblyName = assemblyName, FilePath = projectFilePath, Hierarchy = hierarchy as IVsHierarchy, ProjectGuid = projectGuid, From c2388e73b4af2edc147f97c85dee75e5ffdd1627 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Fri, 21 May 2021 11:42:36 -0700 Subject: [PATCH 4/5] Fix comment that says we aren't using a parameter that's critical --- .../ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs index df0c894e5967f..bae5d127b04f7 100644 --- a/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs +++ b/src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContextFactory.cs @@ -31,7 +31,7 @@ internal interface IWorkspaceProjectContextFactory /// Unique name for the project. /// Full path to the project file for the project. /// Project guid. - /// Obsolete. The argument is ignored. + /// The IVsHierarchy for the project; this is used to track linked files across multiple projects when determining contexts. /// Initial project binary output path. Task CreateProjectContextAsync( string languageName, From d72f606222d5fddde3298acdd927192a911bdcd6 Mon Sep 17 00:00:00 2001 From: Fred Silberberg Date: Thu, 27 May 2021 14:41:29 -0700 Subject: [PATCH 5/5] Add API Review Process Documentation (#53396) * Add API Review Process Documentation We're currently working on making a more formal API review process for Roslyn. This PR adds documentation to the repo on how that process works, and the general expectations for how the process should work. Much of this documentation is copied from dotnet/runtime, and modified to fit the repo. Co-authored-by: Youssef Victor Co-authored-by: Joseph Musser Co-authored-by: Joey Robichaud --- .github/CODEOWNERS | 4 ++ .github/ISSUE_TEMPLATE/api-suggestion.md | 56 ++++++++++++++++++++++++ README.md | 1 + docs/area-owners.md | 28 ++++++++++++ docs/contributing/API Review Process.md | 46 +++++++++++++++++++ 5 files changed, 135 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/api-suggestion.md create mode 100644 docs/area-owners.md create mode 100644 docs/contributing/API Review Process.md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 90960c656649f..518dc286e1e3f 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -27,3 +27,7 @@ src/VisualStudio/ @dotnet/roslyn-ide src/VisualStudio/LiveShare @dotnet/roslyn-ide @daytonellwanger @srivatsn @aletsdelarosa src/Workspaces/ @dotnet/roslyn-ide +src/Compilers/**/PublicAPI.Unshipped.txt @dotnet/roslyn-api-owners +src/Workspaces/**/PublicAPI.Unshipped.txt @dotnet/roslyn-api-owners +src/Features/**/PublicAPI.Unshipped.txt @dotnet/roslyn-api-owners +src/EditorFeatures/**/PublicAPI.Unshipped.txt @dotnet/roslyn-api-owners diff --git a/.github/ISSUE_TEMPLATE/api-suggestion.md b/.github/ISSUE_TEMPLATE/api-suggestion.md new file mode 100644 index 0000000000000..5f6beb1283322 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/api-suggestion.md @@ -0,0 +1,56 @@ +--- +name: API proposal +about: Propose a change to the public API surface +title: '' +labels: [Feature Request, Concept-API] +assignees: '' + +--- + +## Background and Motivation + + + +## Proposed API + + + +```diff +namespace Microsoft.CodeAnalysis.Operations +{ + public class ISwitchExpressionOperation + { ++ public bool IsExhaustive { get; } + } +``` + +## Usage Examples + + + +## Alternative Designs + + + +## Risks + + diff --git a/README.md b/README.md index 4c9cfcf9a8a02..248a13804b4e5 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,7 @@ If you are interested in fixing issues and contributing directly to the code bas - [Submitting pull requests](https://github.com/dotnet/roslyn/blob/main/CONTRIBUTING.md) - Finding a bug to fix in the [IDE](https://aka.ms/roslyn-ide-bugs-help-wanted) or [Compiler](https://aka.ms/roslyn-compiler-bugs-help-wanted) - Finding a feature to implement in the [IDE](https://aka.ms/roslyn-ide-feature-help-wanted) or [Compiler](https://aka.ms/roslyn-compiler-feature-help-wanted) +- Roslyn API suggestions should go through the [API review process]() ### Community diff --git a/docs/area-owners.md b/docs/area-owners.md new file mode 100644 index 0000000000000..76c05e21581a2 --- /dev/null +++ b/docs/area-owners.md @@ -0,0 +1,28 @@ +# Pull Requests Tagging + +If you need to tag folks on an issue or PR, you will generally want to tag the owners (not the lead) for [area](#areas) to which the change or issue is closest to. + +## Areas + +| Area | Lead | Owners | +|----------------------------|---------|--------------------------------| +|api-Syntax |@jaredpar|@CyrusNajmabadi @333fred | +|api-Symbols |@jaredpar|@AlekseyTs | +|api-SemanticModel |@jaredpar|@AlekseyTs @333fred | +|api-IOperation |@jaredpar|@333fred | +|api-SourceGenerators |@jaredpar|@chsienki | +|api-Analyzers |@jaredpar|@mavasani @333fred | +|api-CodeFixes (Refactoring) |@jinuj |@mavasani @sharwell | +|api-Completion |@jinuj |@genlu @CyrusNajmabadi | +|api-Debugging |@jinuj |@tmat @davidwengier | +|api-EditAndContinue |@jinuj |@tmat @davidwengier | +|api-EmbeddedLanguages |@jinuj |@CyrusNajmabadi @joerobich | +|api-Formatting |@jinuj |@sharwell @joerobich | +|api-LSP |@jinuj |@dibarbet @jasonmalinowski | +|api-MetadataAsSource |@jinuj |@sharwell @jasonmalinowski | +|api-Navigation |@jinuj |@CyrusNajmabadi @dibarbet | +|api-QuickInfo |@jinuj |@sharwell @jasonmalinowski | +|api-SignatureHelp |@jinuj |@sharwell @jasonmalinowski | +|api-Structure (Outlining) |@jinuj |@CyrusNajmabadi @jasonmalinowski| +|api-Tagging (Classification)|@jinuj |@CyrusNajmabadi @joerobich | +|api-Workspace |@jinuj |@jasonmalinowski @tmat | diff --git a/docs/contributing/API Review Process.md b/docs/contributing/API Review Process.md new file mode 100644 index 0000000000000..43896603d3607 --- /dev/null +++ b/docs/contributing/API Review Process.md @@ -0,0 +1,46 @@ +# API Review Process + +.NET has a long-standing history of taking API usability extremely seriously. Thus, we generally review every single API that is added to the product. This page discusses how we conduct design reviews for the Roslyn components. + +## Which APIs should be reviewed? + +The rule of thumb is that we (**dotnet/roslyn**) review every public API that is being added to this repository in the CodeAnalysis (compiler), Workspaces, Features, and EditorFeatures libraries. APIs in the LanguageServices dlls are not usually reviewed as they are not intended for general consumption; they are for use in Visual Studio, and we don't have a compat bar for these APIs. + +## Steps + +1. **Requester files an issue**. The issue description should contain a speclet that represents a sketch of the new APIs, including samples on how the APIs are being used. The goal isn't to get a complete API list, but a good handle on how the new APIs would roughly look like and in what scenarios they are being used. Please use [this template](https://github.com/dotnet/roslyn/issues/new?template=api-suggestion.md). The issue should have the labels `Feature Request` and `Concept-API`. [Here](https://github.com/dotnet/roslyn/issues/53410) is a good example of a strong API proposal. + +2. **We assign an owner**. We'll assign a dedicated owner from our side that sponsors the issue. This is usually [the area owner](../area-owners.md#areas) for which the API proposal or design change request was filed. + +3. **Discussion**. The goal of the discussion is to help the assignee to decide whether we want to pursue the proposal or not. In this phase, the goal isn't necessarily to perform an in-depth review; rather, we want to make sure that the proposal is actionable, i.e. has a concrete design, a sketch of the APIs and some code samples that show how it should be used. If changes are necessary, the owner will set the label `api-needs-work`. To make the changes, the requester should edit the top-most issue description. This allows folks joining later to understand the most recent proposal. To avoid confusion, the requester can maintain a tiny change log, like a bolded "Updates:" followed by a bullet point list of the updates that were being made. When you the feedback is addressed, the requester should notify the owner to re-review the changes. + +4. **Owner makes decision**. When the owner believes enough information is available to make a decision, they will update the issue accordingly: + + * **Mark for review**. If the owner believes the proposal is actionable, they will label the issue with `api-ready-for-review`. + * **Close as not actionable**. In case the issue didn't get enough traction to be distilled into a concrete proposal, the owner will close the issue. + * **Close as won't fix as proposed**. Sometimes, the issue that is raised is a good one but the owner thinks the concrete proposal is not the right way to tackle the problem. In most cases, the owner will try to steer the discussion in a direction that results in a design that we believe is appropriate. However, for some proposals the problem is at the heart of the design which can't easily be changed without starting a new proposal. In those cases, the owner will close the issue and explain the issue the design has. + * **Close as won't fix**. Similarly, if proposal is taking the product in a direction we simply don't want to go, the issue might also get closed. In that case, the problem isn't the proposed design but in the issue itself. + +5. **API gets reviewed**. The group conducting the review is called *RAR*, which stands for *Roslyn API Reviewers*. In the review, we'll take notes and provide feedback. After the review, we'll publish the notes in the [API Review repository](https://github.com/dotnet/apireviews) and at the end of the relevant issue. Multiple outcomes are possible: + + * **Approved**. In this case the label `api-ready-for-review` is replaced with `api-approved`. + * **Needs work**. If we believe the proposal isn't ready yet, we'll replace the label `api-ready-for-review` with `api-needs-work`. + * **Rejected**. If we believe the proposal isn't a direction we want to pursue, we'll simply write a comment and close the issue. + +## Review schedule + + There are three methods to get an API review (this section applies to API area owners, but is included for transparency into the process): + +* **Get into the backlog**. Generally speaking, filing an issue in `dotnet/roslyn` and applying the label `api-ready-for-review` on it will make your issue show up during API reviews. The downside is that we generally walk the backlog oldest-newest, so your issue might not be looked at for a while. Progress of issues can be tracked via . +* **Fast track**. If you need to bypass the backlog apply both `api-ready-for-review` and `blocking`. All blocking issues are looked at before we walk the backlog. +* **Dedicated review**. If an issue you are the area owner for needs an hour or longer, send an email to roslynapiowners and we book dedicated time. We also book dedicated time for language feature APIs: any APIs added as part of a new language feature need to have a dedicated review session. Rule of thumb: if the API proposal has more than a dozen APIs, the APIs have complex policy, or the APIs are part of a feature branch, then you need 60 min or more. When in doubt, send mail to roslynapiowners. + +The API review board currently meets every 2 weeks. + +## Pull requests + +Pull requests against **dotnet/roslyn** that add new public API shouldn't be submitted before getting approval. Also, we don't want to get work in progress (WIP) PR's. The reason being that we want to reduce the number pending PRs so that we can focus on the work the community expects we take action on. + +If you want to collaborate with other people on the design, feel free to perform the work in a branch in your own fork. If you want to track your TODOs in the description of a PR, you can always submit a PR against your own fork. Also, feel free to advertise your PR by linking it from the issue you filed against **dotnet/roslyn** in the first step above. + +For **dotnet/roslyn** team members, PRs are allowed to be submitted and merged before a full API review session, but you should have sign-off from the area owner, and the API is expected to be covered in a formal review session before being shipped. For such issues, please work with the API area owner to make sure that the issue is marked blocking appropriately so it will be covered in short order.