Address param-nullchecking public API design review #58987
Conversation
ghost
added
the
| // We never expect multiple declarations for a parameter symbol. | ||
| // Partial method parameters are distinct symbols on each method declaration | ||
| Contract.ThrowIfTrue(syntaxRefs.Length > 1); | ||
| return syntaxRefs.SingleOrDefault() is SyntaxReference syntaxReference |
There was a problem hiding this comment.
SingleOrDefault will throw when there are multiple references. taht woudl be bad.
There was a problem hiding this comment.
I did this because I really do expect a parameter to never have multiple syntax references. Is there a different way of handling this scenario you would prefer? For example, just take the first reference if any, check if any of the references have the !! token, etc.
There was a problem hiding this comment.
Is there a different way of handling this scenario you would prefer? For example, just take the first reference if any, check if any of the references have the !! token, etc.
Checking if any references have !! seems reasonable to me.
There was a problem hiding this comment.
I think it's important to know if in any case (even in future), we can have multiple syntax locations for a parameter. I'd add a Debug.Assert in the base parameter symbol with a comment explaining that this API is assuming a single syntax reference, or no references at all.
There was a problem hiding this comment.
I think it's important to know if in any case (even in future), we can have multiple syntax locations for a parameter. I'd add a
Debug.Assertin the base parameter symbol with a comment explaining that this API is assuming a single syntax reference, or no references at all.
This was my inclination as well. However, I think Debug.Assert will realistically only end up catching the issue if an editor feature test is added which exercises such a new parameter symbol while also using this new API.
There was a problem hiding this comment.
@RikkiGibson I mean adding the assert in ParameterSymbol constructor
There was a problem hiding this comment.
Sorry, I overlooked what you actually said. If we can identify a reasonable central place to assert this assumption, I would be fine with taking it in a separate PR.
...ilitiesAndExtensions/Compiler/VisualBasic/Services/SemanticFacts/VisualBasicSemanticFacts.vb
Show resolved
Hide resolved
…ic/Services/SemanticFacts/VisualBasicSemanticFacts.vb Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
|
@chsienki @dotnet/roslyn-compiler for second review. |
6 participants
Closes #58307
Related to #58335
There were many CFG tests that I felt were obviated. I decided to just keep one of them as a "sanity test" to demonstrate that we don't generate nodes for the null-check.