C#: Add model validation for constructor summary models#21416
C#: Add model validation for constructor summary models#21416owen-mc wants to merge 2 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds validation to the C# model-validation logic to detect constructor summary models that incorrectly specify their output as ReturnValue instead of Argument[this], aligning with the intended constructor modeling semantics (similar to the Java-side validation in #21415).
Changes:
- Add a new validation check that flags constructor summary models using
ReturnValue-prefixed outputs. - Include the new validation message in the aggregated
invalidModelRowpredicate output.
| /** Holds if some row in a MaD flow model appears to contain typos. */ | ||
| query predicate invalidModelRow(string msg) { | ||
| msg = | ||
| [ | ||
| getInvalidModelSignature(), getInvalidModelInput(), getInvalidModelOutput(), | ||
| KindVal::getInvalidModelKind() | ||
| getIncorrectConstructorSummaryOutput(), KindVal::getInvalidModelKind() |
There was a problem hiding this comment.
The doc comment for invalidModelRow says it detects rows that "appear to contain typos", but this predicate now also reports semantic validation issues (for example, incorrect constructor summary outputs and invalid kind values). Please update the comment to reflect the broader scope (e.g., invalid/suspicious model rows) so it stays accurate.
owen-mc
changed the title
| string getIncorrectConstructorSummaryOutput() { | ||
| exists(string namespace, string type, string name, string output | | ||
| summaryModel(namespace, type, _, name, _, _, _, output, _, _, _) and | ||
| type = name and |
There was a problem hiding this comment.
This doesn't work for generics. As opposed to Java, C# doesn't used "erased" types in MaD. This means that a type could be C<T1,T2>, but the constructor name is only C. For types you need to remove everything after (and including) the first < in type before comparing to name.
There was a problem hiding this comment.
Good catch! I've added a commit to address this (in a different way than you suggested, which shows that I didn't even read your whole comment 😬 ). Again, I tested it by modifying an existing model to have the problem and checking that it was found by the test.
The output column of a summary model for a constructor should use
Argument[this]instead ofReturnValue.This didn't find anything, but it did for java (#21415). I also manually tested it by changing a constructor summary model to use
ReturnValueand it was correctly identified when I rancsharp/ql/test/library-tests/dataflow/external-models/validatemodels.ql.