-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix API diff tool crash when no attribute exclusion files are specified #51013
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
base: main
Are you sure you want to change the base?
Conversation
… no files specified Co-authored-by: ericstj <[email protected]>
Co-authored-by: ericstj <[email protected]>
-eattr
makes it fail to load non-existent AttributesToExclude.txt fileThere 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.
Review the tests in https://github.com/dotnet/sdk/blob/main/test/Microsoft.DotNet.ApiDiff.Tests/Diff.Attribute.Tests.cs and be sure to add any necessary tests to cover the default behavior.
@copilot please address all feedback
{ | ||
private static readonly string AttributesToExcludeDefaultFileName = "AttributesToExclude.txt"; | ||
|
||
private static readonly string[] DefaultAttributesToExclude = |
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.
Looks like this is not used anywhere
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.
Fixed in commit c7a4ef9. Removed the unused DefaultAttributesToExclude
array from Program.cs completely.
T:System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute | ||
T:System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute | ||
T:System.Windows.Markup.ContentWrapperAttribute | ||
T:System.Windows.TemplatePartAttribute |
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.
Lets delete this file entirely, as well as all the instances of the place this file is copied and read. https://github.com/search?q=repo%3Adotnet%2Fsdk%20AttributesToExclude.txt&type=code
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.
Fixed in commit c7a4ef9. Deleted AttributesToExclude.txt file entirely and removed all references from both project files (.csproj files updated to remove the file copy configuration).
|
||
Option<FileInfo[]?> optionFilesWithAttributesToExclude = new("--attributesToExclude", "-eattrs") | ||
{ | ||
Description = $"An optional array of filepaths, each containing a list of attributes to exclude from the diff. Each file should contain one API full name per line. You can either modify the default file '{AttributesToExcludeDefaultFileName}' to add your own attributes, or include additional files using this command line option.", |
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.
Let's remove mention of AttributesToExcludeDefaultFileName and update the documentation to mention we have some defaults we apply and specifying this option will replace those with the ones in file provided. Mention the the format of the file is docID T:Namespace.TypeName
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.
Fixed in commit c7a4ef9. Updated the option description to mention that defaults are applied when not specified, and that files should contain docIDs in the format T:Namespace.TypeName
. Removed reference to AttributesToExcludeDefaultFileName.
beforeEVCS.Value is LiteralExpressionSyntax beforeLes && afterEVCS.Value is LiteralExpressionSyntax afterLes) | ||
{ | ||
return beforeLes.Token.ValueText.CompareTo(afterLes.Token.ValueText); | ||
return beforeLes.Token.ValueText.CompareTo(afterLes.Token.ValueText); |
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.
Revert all unrelated changes in this file.
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.
Fixed in commit c7a4ef9. Reverted all unrelated formatting changes in MemoryOutputDiffGenerator.cs by checking out the file from the commit before my changes, then applied only the necessary logic for default attribute handling.
<PackageJsonAuthor Condition="$(PackageJsonAuthor) == ''"></PackageJsonAuthor> | ||
<PackageJsonLicense Condition="$(PackageJsonLicense) == ''">MIT</PackageJsonLicense> | ||
</PropertyGroup> | ||
</Project> No newline at end of file |
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.
Revert this file as well as .DefaultPatchVersionForAspNetCoreApp2_1
and .BundledAspNetCoreVersion
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.
Fixed in commit c7a4ef9. Reverted all unrelated test asset files (.BundledAspNetCoreVersion, .DefaultPatchVersionForAspNetCoreApp2_1, package.g.props, and nuget.exe) by checking them out from the commit before my changes.
…s, and apply defaults to both generators Co-authored-by: ericstj <[email protected]>
Fixed in commit c7a4ef9. Added comprehensive test |
The
apidiff
tool was crashing with aFileNotFoundException
when run without specifying the-eattrs
parameter because it attempted to load a non-existentAttributesToExclude.txt
file from the current working directory.Problem
When users ran the tool without attribute exclusion files:
The tool would fail with:
Solution
This PR implements a robust default attribute exclusion mechanism:
-eattrs
parameter is provided, eliminating any file dependencyAttributesToExclude.txt
file entirely and all references to itDefault Attributes Excluded
The tool now excludes these attributes by default:
T:System.AttributeUsageAttribute
T:System.ComponentModel.EditorBrowsableAttribute
T:System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute
T:System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute
T:System.Windows.Markup.ContentWrapperAttribute
T:System.Windows.TemplatePartAttribute
Changes Made
CollectAttributesFromFilesOrDefaults()
method that uses built-in defaults when no files are specifiedGetAttributesToExcludeOrDefaults()
method to ensure consistent default behavior in testsT:Namespace.TypeName
).csproj
filesTesting
DefaultBehaviorExcludesDefaultAttributes
test validates the default exclusion behaviorSuppressAllDefaultAttributesUsedByTool
test uses hardcoded defaults instead of file-eattrs
parameterFixes the issue where users couldn't run the API diff tool in directories without an
AttributesToExclude.txt
file by completely eliminating the file dependency.Original prompt
This section details on the original issue you should resolve
<issue_title>Running API diff without specifying
-eattr
makes it fail to load non-existent AttributesToExclude.txt file</issue_title><issue_description>When not specifying this parameter, it should either use some pre-defined internal attributes to exclude, or not exclude anything. Loading a non-existent file is busted.
T:System.AttributeUsageAttribute
T:System.ComponentModel.EditorBrowsableAttribute
T:System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute
T:System.D...
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.