-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix ArgumentsSource on external types not working if the argument type is not primitive #2820
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
I thought sample benchmark IntroArgumentsSource.cs also needs to be fixed. Currently IntroArgumentsSource benchmark failed with following errors.
|
Thanks, nice catch. Should be fixed now! |
I'm trying to understand what was broken before. When I run the tests, they succeed. Can you add tests that failed previously that pass with this change? |
What is currently broken is that if you try to use
I address this in the PR description as well. I was also confused by the tests passing, since if you create an example project using my example code it does not work. Does our test infra not currently check the generated code at all? If it does, why did it miss this compiler error, and if it doesn't, how should we go about adding that capability? Before I add tests to this PR, I was hoping somebody with knowledge about our test infra would chime in with some insight. Such a person giving quick answers to the above questions would save me hours of investigating our very complicated test infra myself, which would be nice since it already took me hours of investigation to figure out the cause of this issue and figure out how to fix it. It seems like you didn't understand the information in the PR description. I'm sorry if I presented the information unclearly. Do you have any advice for how I can make my writing easier to understand? |
There doesn't seem to be anything wrong with the test infrastructure. I copied the existing tests to a new benchmark project, and they still succeed. It appears to be due to the arg type |
Catches some issues not previously caught.
Aha, you're completely correct. Thanks a lot! The primitive types got inlined as literals in the generated code which is why the bug wasn't triggered by them. I added some more test cases that use non-primitive types. I've confirmed that the new tests fail before this PR and pass after this PR. I've also updated the PR title and description to reflect this improved understanding of the bug. |
Thanks. Can you also update the ParamsSource tests? |
Done 😎 |
Co-authored-by: Tim Cassell <[email protected]>
Followup to #2748. As of BDN 0.15.2, you can't use
[ArgumentsSource]
with an external type providing the arguments if the argument type is not primitive. This happens because the generated code has compiler errors.The issue
If you have code like this:
Then BDN currently generates code like this:
The problem is that
MethodACases
is not in the type that the generated class (Runnable_0
) inherits from, it's in an external type (DiagnosticsCases
). So there's a compiler error here ("error CS0103: The name 'MethodACases' does not exist in the current context")This only happens if the argument type is non-primitive (like
MethodACase
). If it's a primitive type (likeint
) then the argument value is inlined as a literal, instead of usingParameterExtractor
.The fix
In this example, we need to put the fully qualified type name before the method call. The last line of the generated code should instead read:
However, we can't always add the fully qualified type name, because
ArgumentsSource
can also be used with non-static methods/properties.So, this PR checks if
ArgumentsSource
is being used with a static type. If so, the fully qualified type name is added at this location; otherwise, the generated code is unchanged compared to before this PR.I am worried about external instance methods
Using
ArgumentsSource
for non-primitive arguments with different kinds of members:As you can see from this helpful table, this PR fixes one broken case but there is still one broken case remaining. How best to fix this? Should the generated code create an instance of the member's type, and call the member from the new instance?
To be honest, I think there's no benefit to supporting instance members at all. @timcassell also dislikes that we support instance members and we discussed it a bit here: #2744 (comment).
Maybe we should take this opportunity to just drop support for instance members in
ArgumentsSource
andParamsSource
?Other things I am worried about