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

Misleading indent var warn #25207

Merged
merged 5 commits into from
Jun 11, 2024

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Jun 10, 2024

This was caused by the incorrect location reporting for variables. If the code above is 4-indented, and then location of the name is used, that name is also 4-indented, code flags as misleading.

As an example:

      for 1..10 do
          writeln("Hello, world!");
      var unrelated = "hi";

Note that unrelated and writeln happen to be aligned, which is sufficient for the false positive.

Reported by @alvaradoo -- thanks!

The solution is to steal the logic from IncorrectIndentation that avoids AST nodes that report their locations oddly.

Reviewed by @jabraham17 -- thanks!

Testing

  • start_test test/chplcheck.

This was caused by the incorrect location reporting for variables.
If the code above is 4-indented, and then location of the name is used,
that name is also 4-indented, which flags as misleading.

The solution is to steal the logic from IncorrectIndentation that avoids
AST nodes that report their locations oddly.

Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
This time, Jade, it did get caught by the CI check.

Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
@DanilaFe DanilaFe merged commit c84a1de into chapel-lang:main Jun 11, 2024
8 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.

2 participants