Skip to content

Improved Diagnostic for C-style for loop #1540

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

Closed
wants to merge 3 commits into from

Conversation

CippoX
Copy link
Contributor

@CippoX CippoX commented Apr 14, 2023

Improved Diagnostic for C-style for loop using getTokens introduced by #1464

@CippoX CippoX requested a review from ahoppen as a code owner April 14, 2023 08:42
@kimdv
Copy link
Contributor

kimdv commented Apr 14, 2023

@swift-ci please test

@CippoX CippoX marked this pull request as draft April 14, 2023 13:31
@CippoX
Copy link
Contributor Author

CippoX commented Apr 14, 2023

I have to think about the right approach, I think getTokens cannot be used

Syntax(node.unexpectedBetweenWhereClauseAndBody),
Syntax(unexpectedCondition),
] as [Syntax?]).compactMap({ $0 }),
highlights: (getTokens(between: firstToken, and: lastToken).map { Optional(Syntax($0)) }).compactMap({ $0 }),
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to explicitly spell out Optional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about replicating as [Syntax?],
However, the test failing is testColoringWithHighlights(). The problem seems to be that with getTokens we are creating an array of tokens, which is not what was happening previously. Do you think I should find a way to walk through node's children?

@CippoX
Copy link
Contributor Author

CippoX commented Apr 14, 2023

What about this solution? Obviously, it's very primitive, if you think that's the right approach I'll refine it

@ahoppen
Copy link
Member

ahoppen commented Apr 14, 2023

If we need to write a new function for this, I don’t think it’s worth it. The C-style for loop diagnostic is a bit of a weirdo anyway and unless a new use case for getNodes(between:and:) comes up, I don’t think it’s worth introducing it.

@CippoX
Copy link
Contributor Author

CippoX commented Apr 14, 2023

@ahoppen seems reasonable, what about this last attempt? Otherwise, I'll just close the PR

@ahoppen
Copy link
Member

ahoppen commented Apr 14, 2023

This makes implicit assumptions about the number of children of a for loop that we are not interested in, which might change if we changed the structure of for loops. I’d just close the PR, sorry.

@CippoX CippoX closed this Apr 14, 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