Semgrep: C# support

[mackowski]

I'm opening a ticket to indicate interest in support for the C# Language.
I can help with developing it if you can point me how to start and where to look.

[dlukeomalley]

Thank you for filing this @mackowski. Adding @aryx and @nbrahms to discuss what adding support for a new language looks like. We'd love to get more folks involved there.

[aryx]

Are you familiar with OCaml? the only remaining part to handle C# needs to be written in OCaml (in semgrep/semgrep-core/parsing/ like the Parse_ruby_tree_sitter.ml in this directory for example, but here it would be a Parse_csharp_tree_sitter.ml. We can generate the boilerplate for this file, but we still need to convert that in the generic AST (defined in pfff/h_program-lang/AST_generic.ml)

[mackowski]

I do not know OCaml, but it seems to be fun to learn it. Anyway, as I did not do it before I do not know how much time is needed to do it. I am ready to learn something new and help you with this project, but I also do not want to be a roadblock.
If you can generate the boilerplate in a new branch, I can try to start writing mapping, I will use other languages as examples. If you have capacity and willingness to do this faster do not wait for me, I will review the code to understand it better.

[aryx]

Sounds great! @mjambon can you add csharp in ocaml-tree-sitter-lang? I'll create the Parse_csharp_tree_sitter.ml boilerplate with a few cases filled in as a starting point.

[mjambon]

@aryx I just added csharp to ocaml-tree-sitter-lang. I parsed one sample program successfully on the first try but didn't run large-scale stats.

[aryx]

Ok, I'll create the Parse_csharp_tree_sitter.ml boilerplate with a few cases filled in as a starting point.

[ievans]

@aryx looks like you added the basic parser; did you want to follow up with @mackowski ?

[aryx]

No I didn't. I guess I need to add a few simple cases to be a better starting point and maybe some documentation on where to find the relevant AST definitions.

[aryx]

The main work @mackowski is to replace all the todo in https://github.com/returntocorp/semgrep/blob/develop/semgrep-core/parsing/Parse_csharp_tree_sitter.ml with the relevant construct defined in https://github.com/returntocorp/pfff/blob/master/h_program-lang/AST_generic.ml
Not trivial, but not impossible :)

[mackowski]

looks good! Next week I will be on a small trip without my laptop but after that I will start working on that :)

[Sjord]

I have started an attempt.

[Sjord]

I can't find anything in AST_Generic to represent an expression with an infix operator, such as 1 + 2. I would expect expr * operator * expr. There is AssignOp, but as I understand, that is for a += 1. Am I missing something?

[aryx]

Yes you need to convert that in a Call (IdSpecial (Op Plus), e1, e2)
You can look in the codebase of pffff for js_to_generic.ml, or java_to_generic.ml to see how we convert those languages to the generic AST.

[aryx]

Feel free to ask many many questions @Sjord. We know AST_generic.ml is not very well commented, so we know it's tedious to write those converters from tree-sitter CST to AST_generic.ml
Fantastic job btw, this looks great.

[Sjord]

C# has multidimensional arrays:

        // Three-dimensional array.
        int[, ,] array3D = new int[,,] { { { 1, 2, 3 }, { 4, 5, 6 } },
                                        { { 7, 8, 9 }, { 10, 11, 12 } } };
        // The same array with dimensions specified.
        int[, ,] array3Da = new int[2, 2, 3] { { { 1, 2, 3 }, { 4, 5, 6 } },
                                            { { 7, 8, 9 }, { 10, 11, 12 } } };

        // Accessing array elements.
        System.Console.WriteLine(array2D[0, 0]);

I was thinking of representing this as a TyArray with a Tuple, so that writing array2D[0, 0] is actually accessing array2D with the tuple (0, 0).

However, this presents a problem when parsing something like this:

int[, ,] array3D

This is an array with a tuple of (nothing, nothing, nothing), and that can't be represented as a Tuple as far as I know. Any ideas?

Edit: an alternative is to treat it as an array of arrays. C# also has those, but maybe it's not so bad if we mix up jagged arrays with multidimensional arrays.

[aryx]

Hmm, yes not super easy to represent right now. We could extend the generic AST at some point to better handle those.
In the mean time when a construct does not fit, you can use one of the OtherXxx constructor and Ox_Todo,
for example here could return for those multidimensional array something like
OtherType (OT_Todo, [T (TyArray ((lbracket_tok, None, rbracket_tok), type_elt)])

And later when we improve the generic AST we can transform those OtherType in something more precise.

[aryx]

How are things progressing @Sjord ? Anything we can do to help?
Do you think we could merge what you wrote?

[Sjord]

Progress is slow but steady. The parser can currently parse most common features. We started with 329 todo's. I have resolved 201 of those, and added another 30. The TODO's I added are mostly when I am not sure if what I'm doing is the correct way.

I ran into a couple of upstream issues in treesitter. The treesitter developers are very fast and helpful with resolving these, but I could use your help to get these fixes into semgrep-core.

I am currently only working on the parser, and testing by running semgrep-core -dump_ast. Presumably other parts of semgrep also need some work before semgrep works on C#. I think semgrep currently doesn't even call this parser. I could use some help with that.

The parser is currently half-finished. We could merge it in the current state. It wouldn't break anything, but it is not really complete. Do you have a review process? Perhaps it is a good idea to start reviewing already, before the whole thing is done.

[aryx]

Fantastic! We can definitely help later to update to the latest tree-sitter-csharp and also to extend the grammar to allow semgrep-specific constructs (metavariables, ellipsis). In the mean time, I think the easiest way to go forward and for you to make a Pull Request on our repo so we can review it and merge it.

[aryx]

@dlukeomalley does Sjord need to sign some CLA agreement?

[Sjord]

In declaration_expression, I return a PatVar, which needs a type:

PatVar of type_ * (ident * id_info) option

However, if you have C# like this:

foreach (var item in values)

The variable item doesn't really have a type. So far I have mapped var to None, where a type option is allowed.

Any ideas? Should I map var item to a PatId? Should I map int item to a PatVar, or a PatTyped PatId, or something else?

[aryx]

You can use PatId if the iterator variable does not have a type.

[Sjord]

I ran into a couple of upstream issues in treesitter. The treesitter developers are very fast and helpful with resolving these, but I could use your help to get these fixes into semgrep-core.

Could you help with this, @aryx or @mjambon ? I want to use the latest treesitter C# grammar in semgrep-core. Could you perhaps import that from upstream?

[aryx]

@mjambon can you update ocaml-tree-sitter-lang to use the latest tree-sitter-csharp?

[Sjord]

Most common language features are implemented in the treesitter parser. One feature that isn't implemented yet is LINQ queries. I intent to convert LINQ queries to function calls, to Select, OrderBy, etc. This is pretty hard. And I think I've run into an upstream bug with this.

[aryx]

I still count around 60 todo().
Do you want us to finish those?

[Sjord]

No, I'll keep working on this. What I meant was that language features that are commonly used are implemented, and rarely used features are still to do. The result of this is that the parser in its current state can parse most files.

I am only working on Parse_csharp_tree_sitter.ml, which does the the CST -> AST conversion. Once this parser is done, it would be nice if you can integrate it in semgrep, and finish the remainders for C# support in semgrep.

[aryx]

Ok sounds good.
@mjambon could you add the C# grammar in semgrep-grammars/ and start the extension to support $X (we can do ... later).
We also need to imitate the sgrep_spatch_pattern rule we have in the pfff grammars, because this is the first time we will rely on tree-sitter to also parse the pattern.

[aryx]

I've created a separate issue to use the tree-sitter parser to also parse the semgrep C# pattern.

[aryx]

FYI @Sjord, I made a big refactoring of AST_generic.stmt in https://github.com/returntocorp/semgrep/pull/2289 to make stmt a record. I made an helper function AST_generic.s to help build stmt from a stmt_kind.
I've refatored Parse_csharp_tree_sitter.ml but if you have some unpushed branch I suggest you to rebase from develop.

[Sjord]

How should async/await be mapped? I think async should be a keyword_attribute on methods, and await a special used in Call. Does that make sense?

[aryx]

there is a Await of tok * expr in AST_generic and an Async keyword_attribute

[aryx]

Ok we're getting really close to C# support in Semgrep!

[aryx]

Hopefully we can release in the next version of semgrep next week (0.37)

[aryx]

We are also at around 97% parsing success with tree-sitter, so not bad:
https://dashboard.semgrep.dev/metric/semgrep.core.c-sharp.parse.pct

[cfguimaraes]

@Sjord congratulations, you have done a lot, the community praises you.
I'm here to offer my help.
I see that the work you have done is far away from my knowledge, but I believe I can help with testing, or if some suggest, I could learn to help.

P.S. Sorry for my English, I'm brazilian guy that made a cheaper course :)

[Sjord]

Thanks, @cfguimaraes. You can help me improve the parser by creating small C# test files that have a construct that is currently not implemented. There are two ways to find those:

1. Search in Parse_csharp_tree_sitter.ml for todo env. The first occurence is here, in a function called attribute_target_specifier. Search in the docs how attribute target specifiers work, and create a little C# file similar to the files here.
2. Try to parse actual C# files until you encounter a file that can't be parsed correctly. Then reduce it to a minimal test case.

You can run semgrep-core -dump_ast -lang cs somefile.cs to check whether the file parses. What you are looking for is:

Fatal error: exception (Failure "not implemented")

If you have a minimal test case, this makes it easier for me to implement correct parsing.

What's also needed is semgrep integration tests, like these but for C#. However, I think C# is currently not sufficiently implemented to make passable tests, and I am not really sure how to write these. @aryx, can you comment on that?

[aryx]

For the semgrep integration tests you can follow https://semgrep.dev/docs/status/#maturity-definitions and try at least the tests for the "Alpha" category. You can imitate the tests for lua/ which has currently the "Alpha" state.
If something does not work we probably need to extend the semgrep-grammar for C# at https://github.com/returntocorp/semgrep-grammars/blob/master/src/semgrep-c-sharp/grammar.js
(and again, to possibly imitate what we did for the other languages in his semgrep-grammars/src/semgrep-xxx directories.

[cfguimaraes]

I've been looking at these links, I believe I can help you right now with option 2, parsing files and making it reproducible.
As I learn more about Semgrep I can write tests, no problem =D
And who knows someday I could help you with the parser.

Talk to you as soon as I found this exception in our codebase (is plenty of scenarios)

[ievans]

@cfguimaraes I am excited that you are interested in helping! If you would like even faster responses, you can also join us on Slack: https://r2c.dev/slack. Most of the developers are there and we are very friendly!

[Sjord]

We really need the latest tree-sitter-c-sharp to make semgrep work correctly, even for alpha functionality. I started updating it in semgrep-grammars, but this broke the build after the PR was merged, and I don't fully understand why.

[aryx]

What is the error message? What is broken?

[Sjord]

The CircleCI image here showed failed instead of passed, but when I tried to reproduce it I got different errors each time. However, I think I solved it in my PR.

[Sjord]

@mjambon, could you perhaps import the latest upstream C# grammar? The version in semgrep-c-sharp is pretty old and contains several serious bugs.

[aryx]

@mjambon I can have a go at it. Good way to test for me the new ocaml-tree-sitter organization.