Skip to content

Block ENC for extension blocks #79883

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 13, 2025
Merged

Block ENC for extension blocks #79883

merged 5 commits into from
Aug 13, 2025

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Aug 12, 2025

For now, we're blocking ENC for extension blocks. Full support will come post RTM, for now we want to make sure that nothing crashes. Contributes to #78959.

@333fred 333fred requested a review from a team as a code owner August 12, 2025 16:57
@333fred
Copy link
Member Author

333fred commented Aug 12, 2025

@jcouv @tmat @RikkiGibson for review please.

333fred
333fred commented Aug 12, 2025
View reviewed changes
src/Features/TestUtilities/EditAndContinue/EditScriptDescription.cs
=> AssertEx.Equal(expected, Edits.Select(e => e.GetDebuggerDisplay()), itemSeparator: ",\r\n", itemInspector: s => $"""
"{s}"
""");
=> AssertEx.Equal(expected, Edits.Select(e => e.GetDebuggerDisplay()), itemSeparator: ",\r\n", itemInspector: static s =>
Copy link
Member Author

@333fred 333fred Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't strictly required, but it does make sure that the baseline is always valid C#, even if it has newlines or contains quotes.

@333fred
Block ENC for extension blocks
69b9b11 
For now, we're blocking ENC for extension blocks. Full support will come post RTM, for now we want to make sure that nothing crashes. Contributes to dotnet#78959.
@333fred 333fred force-pushed the block-enc-extensions branch from 40f72e4 to 69b9b11 Compare August 12, 2025 17:02
@333fred 333fred requested review from RikkiGibson, jcouv and tmat August 12, 2025 17:11
@333fred 333fred added the Feature - Extension Everything The extension everything feature label Aug 12, 2025
jcouv
jcouv approved these changes Aug 12, 2025
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (commit 1)

RikkiGibson
RikkiGibson reviewed Aug 12, 2025
View reviewed changes
src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs
}

[Fact]
public void Extension_ExtensionParameter_RemoveName()
Copy link
Member

@RikkiGibson RikkiGibson Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this test is also adding the name, not removing it

RikkiGibson
RikkiGibson reviewed Aug 12, 2025
View reviewed changes
RikkiGibson
RikkiGibson reviewed Aug 12, 2025
View reviewed changes
RikkiGibson
RikkiGibson reviewed Aug 12, 2025
View reviewed changes
RikkiGibson
RikkiGibson reviewed Aug 12, 2025
View reviewed changes
src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs
{
extension(C c)
{
public static int operator +([A] C a, C b) => 1;
Copy link
Member

@RikkiGibson RikkiGibson Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason a parameter is being renamed at the same time as adding an attribute?

It seems a bit weird that EnC believes the parameters were reordered, but, probably not related to your change.

Copy link
Member Author

@333fred 333fred Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo.

RikkiGibson
RikkiGibson approved these changes Aug 12, 2025
View reviewed changes
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo Julien's test suggestion

@dotnet dotnet locked and limited conversation to collaborators Aug 12, 2025
@dotnet dotnet unlocked this conversation Aug 12, 2025
tmat
tmat reviewed Aug 12, 2025
View reviewed changes
tmat
tmat reviewed Aug 12, 2025
View reviewed changes
tmat
tmat reviewed Aug 12, 2025
View reviewed changes
@tmat
Copy link
Member

tmat commented Aug 12, 2025 •
edited
Loading

Reviewed commit 1.

333fred added 2 commits August 13, 2025 14:49
@333fred
Merge remote-tracking branch 'upstream/main' into block-enc-extensions
246bb4f 
* upstream/main: (87 commits)
  Fix ref safety of implicit calls that might capture refs in the receiver (dotnet#76657)
  Update dependencies from https://github.com/dotnet/dotnet build 278961 Updated Dependencies: System.CommandLine (Version 2.0.0-rc.1.25410.101 -> 2.0.0-rc.1.25411.109)
  Update checklist for adding new language version (dotnet#79881)
  Skip ValidateAllOptions flaky test
  Don't try to load file based projects unless we get a .cs file (dotnet#79844)
  Update package restore error message.
  [main] Source code updates from dotnet/dotnet (dotnet#79862)
  Fix failure to report integration test results when retrying Also fix newly failing tests
  feedback
  More semantic update changes (dotnet#79828)
  Fix flow analysis of extern local functions (dotnet#79741)
  Allow using `/main` with top-level statements (dotnet#79577)
  Delete
  Async
  JTF.run
  Delete comment
  Delete code
  Cancel in flight work
  Simplify
  Fix race condition
  ...
@333fred
Fixup tests after merge main.
118cadb
@333fred 333fred enabled auto-merge (squash) August 13, 2025 22:00
@333fred 333fred merged commit 633a6e3 into dotnet:main Aug 13, 2025
24 of 25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 13, 2025
@333fred 333fred deleted the block-enc-extensions branch August 14, 2025 16:55
This was referenced Aug 19, 2025
Closed
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025
@dotnet-bot dotnet-bot mentioned this pull request Aug 22, 2025
Closed
@davidwengier davidwengier mentioned this pull request Aug 25, 2025
Merged
JoeRobich added a commit to dotnet/roslyn-tools that referenced this pull request Aug 29, 2025
@JoeRobich
Update GitHub PR skip logic (#1501)
7053669 
46 PRs before and 36 after
```diff
+    * [Cache diagnostic analyzer computation](dotnet/roslyn#80045)
+    * [Remove parameter always passed the same value](dotnet/roslyn#80042)
     * [Update doc for IMethodSymbol.IsExtensionMethod](dotnet/roslyn#80016)
     * [Don't cache known-broken compositions](dotnet/roslyn#80021)
     * [Cleanup methods in DiagAnalyzerService](dotnet/roslyn#80013)
     * [Simplify processing of errors reported by the build](dotnet/roslyn#79964)
     * [Additional cleanup of the DiagnosticAnalyzerServier](dotnet/roslyn#80005)
     * [Fix Code Lens around source generated files](dotnet/roslyn#79992)
     * [Remove superflous DiagService api that can be achieved with existing apis](dotnet/roslyn#80007)
     * [Generate `init` accessor for required properties inside `readonly struct`s](dotnet/roslyn#80004)
     * [Remove existing low level diag oop code now that it's all handled at higher levels.](dotnet/roslyn#79994)
     * [Allow large InlineHint ArrayBuilder pooling](dotnet/roslyn#79857)
     * [Reduce allocations obtaining classified spans in ClassifierHelper](dotnet/roslyn#79856)
     * [Compute span diagnostics in oop](dotnet/roslyn#79991)
     * [Allow Razor cohosting to work with non-Razor SDK projects](dotnet/roslyn#79953)
     * [Move computation of deprioritized analyzers to oop](dotnet/roslyn#79989)
     * [EnC: Fix symbol mapping of delegates with indexed name](dotnet/roslyn#79837)
     * [Emit telemetry 'durations' with known radix point '.'](dotnet/roslyn#79988)
     * [Move logic up into DiagService](dotnet/roslyn#79985)
     * [Move the StateManager type up to the DiagnosticService from the DiagnosticIncrementalANalyzer](dotnet/roslyn#79984)
     * [Immediately remote diagnostics call to OOP](dotnet/roslyn#79983)
     * [Build Microsoft.CodeAnalysis.SemanticSearch.Extension ref assembly for use in semantic search queries](dotnet/roslyn#79972)
     * [Only cache compilation if we have the same set of analyzers](dotnet/roslyn#79978)
     * [Delete unused property](dotnet/roslyn#79963)
     * [Update 'use expr body' to be a purely syntactic analyzer](dotnet/roslyn#79979)
     * [Mark 'Use expr body' as a syntax-only fixer](dotnet/roslyn#79971)
     * [♻️ MSBuildWorkspaceDirectory - Fallback to AppContext.BaseDirectory when Assembly Location is empty](dotnet/roslyn#79934)
     * [Merge runtime async support into main](dotnet/roslyn#79833)
     * [Implement "Simplify property accessor" feature](dotnet/roslyn#79754)
-    * Merge main to runtime async branch (PR: [#79961](dotnet/roslyn#79961))
     * [Redo how and when we report source generator telemetry](dotnet/roslyn#79951)
     * [Allow MEF components to supply assembly path resolvers](dotnet/roslyn#79218)
     * [Allow Razor to hook up the source generator in misc files](dotnet/roslyn#79891)
     * [Block ENC for extension blocks](dotnet/roslyn#79883)
     * [Upgrade servicehub.client to fix test source discovery](dotnet/roslyn#79899)
     * [Update package restore error message.](dotnet/roslyn#79876)
-    * Merge main (PR: [#79834](dotnet/roslyn#79834))
-    * Merge main (PR: [#79830](dotnet/roslyn#79830))
     * [Baseline struct lifting tests](dotnet/roslyn#79505)
-    * Merge main to runtime async branch (PR: [#79582](dotnet/roslyn#79582))
-    * Merge main (PR: [#79424](dotnet/roslyn#79424))
-    * Merge main (PR: [#78994](dotnet/roslyn#78994))
-    * Merge main to runtime async branch (PR: [#78740](dotnet/roslyn#78740))
-    * Merge main to runtime async branch (PR: [#78517](dotnet/roslyn#78517))
-    * Merge main to runtime async branch (PR: [#78114](dotnet/roslyn#78114))
-    * Merge main to runtime async branch (PR: [#77700](dotnet/roslyn#77700))
-    * Merge main to runtime async branch (PR: [#77533](dotnet/roslyn#77533))
-    * Merge main to runtime async branch (PR: [#77265](dotnet/roslyn#77265))
```
This was referenced Sep 8, 2025
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

No one assigned

Labels

Area-IDE Feature - Extension Everything The extension everything feature VSCode

Projects

None yet

Milestone

18.0 P1

Development

Successfully merging this pull request may close these issues.

4 participants

@333fred @tmat @RikkiGibson @jcouv