-
-
Notifications
You must be signed in to change notification settings - Fork 226
ref: rename SourceGenerators to Compiler-Extensions #4702
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## version6 #4702 +/- ##
===========================================
Coverage ? 73.19%
===========================================
Files ? 480
Lines ? 17422
Branches ? 3437
===========================================
Hits ? 12752
Misses ? 3820
Partials ? 850 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sentry review |
| using System.Linq; | ||
| using Microsoft.CodeAnalysis; |
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.
The code uses StringBuilder class (sb.Append, sb.AppendLine) but there is no using System.Text; statement at the top of the file. This will cause a compilation error. Add the missing using statement.
Severity: CRITICAL
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/Sentry.Compiler.Extensions/BuildPropertySourceGenerator.cs#L1-L3
Potential issue: The code uses `StringBuilder` class (sb.Append, sb.AppendLine) but
there is no `using System.Text;` statement at the top of the file. This will cause a
compilation error. Add the missing using statement.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
Short: Already included via https://github.com/getsentry/sentry-dotnet/blob/d1aa0f1f902df371df95977fa2ddca64b8fa8d22/GlobalUsings.cs
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.
Long: making Semantic decisions just off of Syntax is pretty much impossible these days in .NET:
- C# 10.0 Global Using Directives
- can be declared in C# per project, e.g.
global using System.Text; - can be declared in MSBuild per project, e.g.
<Using Include="System.Text" />- and then there is also the opposite
<Using Remove="System.Text" />
- and then there is also the opposite
- additionally, both a C# "GlobalUsings.cs" file, as well as MSBuild
<Using/>properties can be imported to multiple projects explicitly via<Import Project=".." />or implicitly viaDirectory.Build.propsorDirectory.Build.targets
- can be declared in C# per project, e.g.
StringBuilder!=StringBuilder- users can declare their own custom
StringBuildertype in a different namespace ... e.g. aSentry.StringBuilder - without a fully-qualified-identifier in the source text, a type may origin from any imported namespace ... although there will be compiler errors for ambiguities, when a non-qualified typename occurs in multiple namespaces that have been imported
- users can declare their own custom
- Source Generators and Interceptors
- with C# Interceptors available, by just looking at C# Syntax we don't definitively know whether the mentioned methods
AppendandAppendLineare actually bound to the instance members ofSystem.Text.StringBuilder, or intercepted (at compile-time) by another ordinary method
- with C# Interceptors available, by just looking at C# Syntax we don't definitively know whether the mentioned methods
- Extension Members
- not really applicable to this comment, but further highlighting that without the "Semantic Model" (by only inspecting the C# Syntax Tree) we can't definitively say which types and members the compiler is binding to ... unless it's one of the predefined C# types like
int,string, and so on
- not really applicable to this comment, but further highlighting that without the "Semantic Model" (by only inspecting the C# Syntax Tree) we can't definitively say which types and members the compiler is binding to ... unless it's one of the predefined C# types like
- Implicit Global Usings
- also not applicable to this comment, but
- with the MSBuild property
<ImplicitUsings>enable</ImplicitUsings>, depending on the Project SDK (e.g.<Project Sdk="Microsoft.NET.Sdk">), a predefined set of Global Usings are automatically emitted
Regarding this comment:
System.Text actually is imported ... via:
BuildPropertySourceGenerator.csis in ProjectSentry.Compiler.Extensions- Project
Sentry.Compiler.Extensionsimplicitly imports the parent directory'sDirectory.Build.props - this
/src/Directory.Build.propsfile explicitly imports the parent directory'sDirectory.Build.props - this
/Directory.Build.propsfile adds aGlobalUsings.csfile to be compiledsentry-dotnet/Directory.Build.props
Lines 79 to 80 in 1ddfa66
<ItemGroup Condition="!$(MSBuildProjectName.Contains('Samples'))"> <Compile Include="$(MSBuildThisFileDirectory)GlobalUsings.cs" />
- this /GlobalUsings.cs includes the Global Using
Line 30 in 1ddfa66
global using System.Text;
alexsohn1126
left a comment
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.
Is there a reason why we are renaming these from SourceGenerators? What parts about it make it more a compiler extension than a source generator?
A Source-Generator is a Compiler-Extension.
and I believe you can also ship a "Completion-Provider" (as in the IDEs auto-completion lists). It's best practice to separate Compiler-Extensions that do require the Workspaces APIs from the ones that do not. Because depending on the Host (the Roslyn Compiler is "just" a library) the Workspaces APIs may or may not have been loaded.
So we want to ship up to two Roslyn-Components/DLLs that include Compiler-Extension: the ones that do not require the Workspaces APIs, and the ones that do. Our preference was to include something like "Compiler" and "Extension" in the name, since these are the documented terms for these Roslyn Components: So we could go with
|
alexsohn1126
left a comment
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.
Thanks for the explanation! Love learning more about .NET 💯
Rename
As discussed here: #4321 (comment)
I noticed that packages
6.0.0-preview.1-prereleaseand6.0.0-preview.2-prereleaseare missing the compiler extensions under/analyzers/dotnet/cs/Sentry.SourceGenerators.dll.I got a fix for that almost ready in a personal project, that I'll apply to Sentry
version6branch tomorrow.While noticing the problem, I thought: might as well do the rename now ... as I am not 100 % certain it's not a breaking change for some scenarios ... so I was thinking about doing this for v6.
#skip-changelog