Skip to content
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

Add support for checksum-based interceptors #72814

Merged
merged 24 commits into from
Apr 15, 2024
Merged

Add support for checksum-based interceptors #72814

merged 24 commits into from
Apr 15, 2024

Conversation

RikkiGibson
Copy link
Contributor

Implementation of #72133

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 30, 2024
CyrusNajmabadi
CyrusNajmabadi reviewed Mar 30, 2024
View reviewed changes
docs/features/interceptors.md Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs Show resolved Hide resolved
docs/features/interceptors.md Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Utilities/InterceptableLocation.cs Show resolved Hide resolved
@RikkiGibson RikkiGibson marked this pull request as ready for review April 1, 2024 21:25
@RikkiGibson RikkiGibson requested review from a team as code owners April 1, 2024 21:25
CyrusNajmabadi

This comment was marked as outdated.

Sign in to view

CyrusNajmabadi
CyrusNajmabadi approved these changes Apr 1, 2024
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Liked everything i saw about the API and the internal impl of the interceptablelocation type. didn't look beyond that.

cston
cston reviewed Apr 6, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Utilities/InterceptableLocation.cs
const int positionIndex = hashIndex + hashSize;
const int positionSize = sizeof(int);
const int displayNameIndex = positionIndex + positionSize;
const int minLength = displayNameIndex;
Copy link
Member

Choose a reason for hiding this comment

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

minLength

Consider inlining, to reduce the number of constants (slightly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer this factoring as it's more self-documenting.

jcouv
jcouv reviewed Apr 8, 2024
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.

Done with review pass (iteration 22)

jcouv
jcouv approved these changes Apr 8, 2024
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 (iteration 22)

@jcouv
Copy link
Member

jcouv commented Apr 8, 2024

Note: the TODO2 will need to be resolved for CI

@RikkiGibson
Copy link
Contributor Author

@cston Please take a look

cston
cston reviewed Apr 11, 2024
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs
var locationSpecifier = model.GetInterceptableLocation(node)!;
Assert.Equal("(7,9)", locationSpecifier.GetDisplayLocation());
AssertEx.Equal("""[global::System.Runtime.CompilerServices.InterceptsLocationAttribute(1, "jB4qgCy292LkEGCwmD+R6FIAAAA=")]""", locationSpecifier.GetInterceptsLocationAttributeSyntax());
}
Copy link
Member

Choose a reason for hiding this comment

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

}

Consider testing intercepting this invocation.

This was referenced Apr 12, 2024
Merged
Merged
@RikkiGibson
Copy link
Contributor Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@RikkiGibson RikkiGibson merged commit 839e1ab into dotnet:main Apr 15, 2024
24 of 27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 15, 2024
@RikkiGibson RikkiGibson deleted the interceptablelocation branch April 15, 2024 19:06
@RikkiGibson RikkiGibson restored the interceptablelocation branch April 15, 2024 19:06
@RikkiGibson RikkiGibson mentioned this pull request Apr 15, 2024
Closed
@RikkiGibson RikkiGibson deleted the interceptablelocation branch April 15, 2024 19:47
This was referenced Apr 18, 2024
Closed
Closed
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
@RikkiGibson RikkiGibson mentioned this pull request Dec 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jjonescz jjonescz jjonescz left review comments

@jaredpar jaredpar jaredpar left review comments

@jcouv jcouv jcouv approved these changes

@cston cston cston approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.11 P1
Development

Successfully merging this pull request may close these issues.
7 participants
@RikkiGibson @jcouv @jaredpar @jjonescz @CyrusNajmabadi @cston @dibarbet