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

Formatter fixes #1840

Merged
merged 20 commits into from
Jun 15, 2023
Merged

Formatter fixes #1840

merged 20 commits into from
Jun 15, 2023

Conversation

petervdonovan
Copy link
Collaborator

@petervdonovan petervdonovan commented Jun 11, 2023

This fixes some of the problems in #1832.

  • (1) address interaction of semicolons with at clauses
  • (2) fix comment duplication after annotations that precede reactor definitions
  • (3) fix removal of the contents of parenthesized list initializer expressions in C target
  • (4) fix lack of idempotency
  • (5) do not put comments that were not in {= =} into the {= =}
  • (6) do not eliminate comments that appear at the start of a code block that is an initializer

Point 4 might be merely a result from point 3, since the previous point results from destructive modification of the ECore model that might somehow violate the assumptions of the line breaker. Therefore, it might be solved; I'm not sure.

I am not sure about point (5). Fixing it would likely result in more special cases in the formatter. I would also like to point out that the current behavior is safe and semantics-preserving because comments that appear right after an opening {= do not seem to be included in the resulting Code ECore object. Instead, I think they are treated as LF-level comments. As a result, a program with the following reaction compiles with the C target:

    reaction(t) -> y {= # this is a test
       lf_set(y, self->count);
       self->count++;
    =}

whereas the following does not:

    reaction(t) -> y {= # this is a test
       # this is another test
       lf_set(y, self->count);
       self->count++;
    =}

Note the usage of a # sign, which is not a valid C comment.

  • (7) do not copy comments to the outside of initializers that use code:
    init_x1 = {= Va_eq * (1.0 - 1.911199519984605 /*a[1]*/ - 0.001916104193780 /*b[1]*/) =},

formats to

    /** a[1] */
    /** b[1] */
    init_x1 = {= Va_eq * (1.0 - 1.911199519984605 /*a[1]*/ - 0.001916104193780 /*b[1]*/) =},

doSwitch is what finds the comments. If you skip doSwitch and call
directly into the code that is specific to the given EObject, you will
drop comments.
There is some black magic happening in the implementation of EList::add
that removes EObjects from their containers when they are added to a new
container. This works around that by just not adding the EObjects to a
container.
This only applies when the comment is in the body of the code block.
According to the grammar, the body does not start until some
tokens appear -- that means non-whitespace, non-comment characters.
@petervdonovan petervdonovan linked an issue Jun 13, 2023 that may be closed by this pull request
@petervdonovan
Copy link
Collaborator Author

petervdonovan commented Jun 13, 2023

I think the only remaining problem is (5).

I do not disagree with Christian on this, but it is not exactly a trivial change (it might take an afternoon to get it right). Therefore, this PR is awaiting a consensus on problem (5).

EDIT: Actually I just realized that I also introduced another issue. Comments are deleted when they appear at the beginning of a code block. This will be fixed easily tomorrow. DONE

This may slow down the formatter. It also is not certain to work
because isEqual could contain mistakes. However, it may substantially
reduce the likelihood that the formatter silently produces wrong
results. It is hard to be sure about this, however, because it is
already rare for the formatter to produce wrong results.
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Thanks a lot for those fixes. They look good to me. I also tested this version of the formatter on the examples repo and it seems to fix the problems that I discovered previously.

I think we should also add tests based on the corner cases that I listed in #1832. We could add new integration tests, but probably it would be better to add specific formatter unit tests like the simple ones we already have in LffCliTest.java

core/src/main/java/org/lflang/ast/ParsingUtils.java Outdated Show resolved Hide resolved
This avoids inlining comments when the target syntactic element is
multiple lines long. In such cases the comment instead gets its own
line.
If a code block begins with a multiline comment, the multiline comment
should still belong to the code block.
@petervdonovan petervdonovan marked this pull request as ready for review June 14, 2023 21:00
@petervdonovan
Copy link
Collaborator Author

I'd like to draw attention to two significant changes other than the bugfixes:

  • The formatter now does an isEqual check to make sure that its output is semantically equivalent to its input
  • Comments are no longer inlined if their target syntactic element is more than one line long.

The latter of these two changes is responsible for the big diff in our tests. The other bugfixes did not affect our tests because they only addressed corner cases that were found in the examples repo.

I believe this is the CRLF/LF thing.
test/C/temp.txt Outdated Show resolved Hide resolved
Copy link
Member

@lhstrh lhstrh 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 to me, modulo the .txt file that I commented on.

@petervdonovan petervdonovan added this pull request to the merge queue Jun 15, 2023
Merged via the queue into master with commit 864ac74 Jun 15, 2023
@petervdonovan petervdonovan deleted the formatter-fixes branch June 15, 2023 08:04
@cmnrd
Copy link
Collaborator

cmnrd commented Jun 15, 2023

Great! Thanks a lot @petervdonovan!

@lhstrh lhstrh added the bugfix label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bugs in formatter
3 participants