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.

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

Fix false-positive errors from xunit/xunit#2826 #170

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

JamesTerwilliger
Copy link
Contributor

This PR fixes a regression introduced by xunit/xunit#2826. It tests to see if the argument to the MemberDataAttribute should be treated as a single argument rather than a params array.

@bradwilson FYI

Copy link
Sponsor

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

Tests look good as defined.

if (type is IArrayTypeSymbol arrayType && arrayType.ElementType.SpecialType == SpecialType.System_Object)
return initializer.Expressions.ToList();

return new List<ExpressionSyntax> { argumentExpression };
Copy link
Sponsor

Choose a reason for hiding this comment

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

@JamesTerwilliger could this be shortcutted by inverting the if on 244 and executing the if between line 231 and 232? then in this case, it does not have to determine initializer. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@viceroypenguin I had it this way thinking that initializer being not null means that it is an array type, and that the semantic model call is likely more expensive. Said differently, the initializer being null is the short-circuit that I want so that the (I believe) expensive call is only done if need be.

Copy link
Sponsor

Choose a reason for hiding this comment

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

Good reasoning; and I'll trust your experience with this code, not having enough experience with touching the semanticModel in analyzers. LGTM

@bradwilson bradwilson changed the title Candidate fix for https://github.com/xunit/xunit/issues/2826 Fix false-positive errors from xunit/xunit#2826 Nov 27, 2023
@bradwilson bradwilson merged commit 45a493e into xunit:main Nov 27, 2023
5 checks passed
@bradwilson
Copy link
Member

Thanks!

@JamesTerwilliger JamesTerwilliger deleted the jamest/bugfix branch November 27, 2023 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants