-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 support for checksum-based interceptors #72814
Merged
Merged
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
3f1a038
Add support for checksum-based interceptors
RikkiGibson 53e0548
Fix broken tests
RikkiGibson f12ce01
use position
RikkiGibson db13fb5
pass cancellation token
RikkiGibson 2cb8bfd
Pass token
RikkiGibson 355728a
Update baseline
RikkiGibson 2394377
update interceptors.md
RikkiGibson 4b147f7
Fix nullable warning
RikkiGibson 231c190
remove reference to line/column encoding
RikkiGibson 1056037
more feedback
RikkiGibson ac01bc4
Update src/Compilers/CSharp/Portable/Utilities/ContentHashComparer.cs
RikkiGibson 282bff3
Address feedback
RikkiGibson 5f85fcb
Apply suggestions from code review
RikkiGibson 8ac441d
Apply diagnostics to attribute name in all cases to reduce churn
RikkiGibson 9763c4f
Add tests to exercise 'interceptable syntactically' concept
RikkiGibson 5bef2bc
address some feedback. Port tests for newly added code paths.
RikkiGibson 499fd18
Decode next to encode
RikkiGibson e1f9273
fix signature
RikkiGibson 13ece53
Address feedback
RikkiGibson b2b30c5
Merge remote-tracking branch 'upstream/main' into interceptablelocation
RikkiGibson 00e90be
Adjust error codes
RikkiGibson 2d65d38
Also test other display location
RikkiGibson e8aed9f
Merge branch 'main' of https://github.com/dotnet/roslyn into intercep…
RikkiGibson 9230536
pack
RikkiGibson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5207,13 +5207,40 @@ protected sealed override ISymbol GetDeclaredSymbolCore(SyntaxNode node, Cancell | |
|
||
CheckSyntaxNode(node); | ||
|
||
if (node.GetInterceptableNameSyntax() is { } nameSyntax && Compilation.TryGetInterceptor(nameSyntax.GetLocation()) is (_, MethodSymbol interceptor)) | ||
if (node.GetInterceptableNameSyntax() is { } nameSyntax && Compilation.TryGetInterceptor(nameSyntax) is (_, MethodSymbol interceptor)) | ||
{ | ||
return interceptor.GetPublicSymbol(); | ||
} | ||
|
||
return null; | ||
} | ||
|
||
#pragma warning disable RSEXPERIMENTAL002 // Internal usage of experimental API | ||
public InterceptableLocation? GetInterceptableLocation(InvocationExpressionSyntax node, CancellationToken cancellationToken) | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
CheckSyntaxNode(node); | ||
if (node.GetInterceptableNameSyntax() is not { } nameSyntax) | ||
{ | ||
return null; | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return GetInterceptableLocationInternal(nameSyntax, cancellationToken); | ||
} | ||
|
||
// Factored out for ease of test authoring, especially for scenarios involving unsupported syntax. | ||
internal InterceptableLocation GetInterceptableLocationInternal(SyntaxNode nameSyntax, CancellationToken cancellationToken) | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
var tree = nameSyntax.SyntaxTree; | ||
var text = tree.GetText(cancellationToken); | ||
var path = tree.FilePath; | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var checksum = text.GetContentHash(); | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
var lineSpan = nameSyntax.Location.GetLineSpan().Span.Start; | ||
var lineNumberOneIndexed = lineSpan.Line + 1; | ||
var characterNumberOneIndexed = lineSpan.Character + 1; | ||
CyrusNajmabadi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return new InterceptableLocation1(checksum, path, nameSyntax.Position, lineNumberOneIndexed, characterNumberOneIndexed); | ||
} | ||
#nullable disable | ||
|
||
protected static SynthesizedPrimaryConstructor TryGetSynthesizedPrimaryConstructor(TypeDeclarationSyntax node, NamedTypeSymbol type) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Was there a discussion of embedding the version number directly in the opaque data string? Maybe
<version>-<data>
?The version number doesn't seem especially useful when I look at an attribute like
[InterceptsLocation(1, "yPU5+1/pMuRHlz+XbnIQwQYAAAARAAAAUHJvZ3JhbS5jcw==")]
Also, consider adding an example of location encoding in the doc to show what it looks like.
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.
It was thought that this gives the ability to change the precise encoding used in the data string, e.g. maybe in the future we want something different than base64, splitting out the version gives us the ability to express that.
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.
Looked again and I think I better understand the suggestion, which was to have a prefix that isn't necessarily encoded the same way. Yes, it is expressible either way. The idea of combining them together for the "opaque encoding" approach didn't come up in API review.
The only reason I can think of to keep it this way is to separate parts that we think should be user-readable from those that are not, when it comes to diagnosing version incompatibility issues (which would arise from compiling later interceptors code in an earlier compiler.)