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

chplcheck: allow ignoring parent/child incorrect indentation using @chplcheck.ignore #25800

Merged

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Aug 22, 2024

This RP allows for disabling the IncorrectIndentation warning for statements that are incorrectly indented relatively to their parent:

@chplcheck.ignore("IncorrectIndentation")
module M {
proc foo() { // would warn; silenced by attribute
  writeln("Hi");
    writeln("There"); // still warns; attribute doesn't globally disable warnings
}
}

This helps the case in which a previously-implicit module is converted into an explicit module.

This code:

proc foo() {
  writeln("Hi");
    writeln("There");
}

Becomes:

module NewMod {
proc foo() { // would warn; silenced by attribute
  writeln("Hi");
    writeln("There"); // still warns; attribute doesn't globally disable warnings
}
}

Now, foo is incorrectly indented. To fix this, the entire file would need to be indented by two spaces, which creates a big diff and may be undesirable. By adding the ability to ignore an individual parent/child indentation violation using @chplcheck.ignore, this PR makes it possible to silence the warnings from explicitly defining NewMod, while preserving all other indentation warnings that had existed in the code previously (or would appear in the code in the future).

This PR also adds the chplcheck attribute to a list of well-known tools by the compiler, in preparation of using this attribute with the standard library.

Reviewed by @jabraham17 -- thanks!

Testing

  • test/chplcheck

…ments

It's unclear how to silence this for sibling statements; using
attributes on either sibling doesn't seem like the right approach.

Signed-off-by: Danila Fedorin <[email protected]>
Copy link
Member

@jabraham17 jabraham17 left a comment

Choose a reason for hiding this comment

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

Looks good!

Can you also open an issue for the feature request we discussed offline, chplcheck.ignoreAll(ruleName) to ignore the rule in all children

frontend/lib/uast/post-parse-checks.cpp Show resolved Hide resolved
test/chplcheck/IncorrectIndentation.chpl Outdated Show resolved Hide resolved
Signed-off-by: Danila Fedorin <[email protected]>
@DanilaFe DanilaFe merged commit 45710bb into chapel-lang:main Aug 22, 2024
8 checks passed
@bradcray
Copy link
Member

Woohoo! Thanks for the quick help here, Daniel!

bradcray added a commit that referenced this pull request Aug 26, 2024
[reviewed by @lydia-duncan]

This adds and updates module index description comments in support of
#25667. Here, I focused on modules I obviously own, key distributions
that were lacking a description, and entries that took up multiple
lines:

* BlockCycDist.chpl
* BlockDist.chpl
* CyclicDist.chpl
* PrivateDist.chpl
* ReplicatedDist.chpl
* StencilDist.chpl
* LayoutCS.chpl
* Math.chpl
* OS.chpl

In order to add documentation for the block and cyclic distributions, I
had to add an explicit `module` keyword. This flipped it from a
prototype module to a production module, popping us into Chapel's
stricter error-handling mode. As a result, I had to add some new
`throws` declarations to dsiSerialRead/Write routines to make them pass.
Happily, since these are internal routines, it is not a breaking change.
(I also removed a commented out and redundant dsiSerialWrite routine in
CyclicDist.chpl).

Adding the `module` keyword (but not doing any other reformatting), I
broke our CI's linting of the file and didn't want to wrestle with
fixing the indentation here. As a result, I applied the new attribute
added by Daniel in #25800 to keep things working for the time being.
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