Skip to content

Conversation

@BoykoAlex
Copy link
Contributor

No description provided.

@martinlippert
Copy link
Member

Hey @BoykoAlex, I very much like the simplicity here, looks great in general. I added a few test cases for this and two of them are failing.

One is around the initial type itself being included in the list of supertypes (which I think wasn't the case before and would result in a different behavior where this method is being called).

The other one is around having the same interface in the hierarchy multiple times, not sure if we want that or not though.

@BoykoAlex
Copy link
Contributor Author

@martinlippert I have excluded the type itself from the hierarchy traversal but the methods such as isAnyInHierarchy and areAllInHierarchy check the type itself as well. I think interface implemented multiple times should likely be traversed as many times as it is met in the hierarchy. Cycles would cause compilation errors... but I think we'd need a test case to see how AST behave in this case.
I see that most tests in ASTUtils are expecting the type itself to be present in the hierarchy... Why don't we rename the the methods to getTypesHierarchyIterator and include the type itself in there? I don't think it is a bad idea... We can even have two variants of the methods: Breadth First and Depth First (queue vs stack)?

@martinlippert
Copy link
Member

@martinlippert I have excluded the type itself from the hierarchy traversal but the methods such as isAnyInHierarchy and areAllInHierarchy check the type itself as well.

+1

I think interface implemented multiple times should likely be traversed as many times as it is met in the hierarchy.

Ok.

Cycles would cause compilation errors... but I think we'd need a test case to see how AST behave in this case

I think I already added a test case for that.

I see that most tests in ASTUtils are expecting the type itself to be present in the hierarchy... Why don't we rename the methods to getTypesHierarchyIterator and include the type itself in there? I don't think it is a bad idea..

Yeah, let's do that. But then, we would need an additional method that returns the subtypes only (I think some symbol providers make use of that). But probably all the "isIn" variants can use this.

We can even have two variants of the methods: Breadth First and Depth First (queue vs stack)?

Interesting idea, but I haven't come across the need for this (yet).

@BoykoAlex
Copy link
Contributor Author

@martinlippert as agreed above + findSuperTypes(...) returns the Set rather than takes as a parameter as well as does not include the passed in type.

@martinlippert
Copy link
Member

Sounds good to me, let's merge! :-)

@martinlippert martinlippert merged commit 3df5d8e into main Apr 1, 2025
4 checks passed
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