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

Add type fields #316

Merged
merged 7 commits into from
Jun 21, 2023
Merged

Add type fields #316

merged 7 commits into from
Jun 21, 2023

Conversation

tamasvajk
Copy link
Collaborator

No description provided.

@tamasvajk tamasvajk marked this pull request as ready for review June 19, 2023 11:44
var a = x is int;
// ^ type.builtin
var a = x is A;
// ^
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not highlighted due to #317.

@@ -16,12 +16,11 @@

[
(implicit_type)
(nullable_type)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this might have been a mistake in the query. I don't think we'd want to color System.Int32? differently than System.Int32 in the below:

void M(System.Int32? b, System.Int32 c) { }

Copy link
Contributor

@hendrikvanantwerpen hendrikvanantwerpen left a comment

Choose a reason for hiding this comment

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

As I've said before, I'm not a big fan of making all elements in a simple list fields. The main reason is that it makes parse trees more verbose and more difficult to read, something one has to do quite often when developing queries. It also goes against my intuition that things are either like records (each field appears at most once) or like collections (all things are similar). I see that it does allow for blanket queries such as (_ type: (_)). And, given that @damieng agrees with this change, I'm not going to block it.

@damieng
Copy link
Collaborator

damieng commented Jun 20, 2023

As I've said before, I'm not a big fan of making all elements in a simple list fields. The main reason is that it makes parse trees more verbose and more difficult to read, something one has to do quite often when developing queries. It also goes against my intuition that things are either like records (each field appears at most once) or like collections (all things are similar). I see that it does allow for blanket queries such as (_ type: (_)). And, given that @damieng agrees with this change, I'm not going to block it.

My experience is mostly with the parsing and tree-sitter side so I when it comes to fields and other magic to light up GH Semantic I'll defer to others.

@tamasvajk
Copy link
Collaborator Author

tamasvajk commented Jun 21, 2023

I've removed the two cases where field names were added in simple lists. I don't have a strong feeling about these, so let's go with the non questionable field names first, and then if we see value in having the others, we'll add them.

I also added some more highlighting test cases to cover the type lists (base list, and type argument list).

@tamasvajk tamasvajk merged commit adfbb69 into tree-sitter:master Jun 21, 2023
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