lingua-franca icon indicating copy to clipboard operation
lingua-franca copied to clipboard

Add capability to generate LF code and create a CLI program for formatting

Open petervdonovan opened this issue 2 years ago • 7 comments

Tasks:

  • [x] Implement missing cases in visitor pattern
  • [x] Implement line wrapping (Billy, Peter) -- (edit: @petervdonovan is making another pass at this)
  • [x] Preserve comments - @petervdonovan
  • [x] Allow the user to disable formatting for specific sections of their code (see comments below) - @petervdonovan
  • [x] Add CI check that applies formatter to all *.lf files in the test directory and checks for equivalence (@lhstrh)
  • [x] Add methods for testing semantic equivalence of AST subtrees
  • [x] Rename org.lflang.lfc to org.lflang.cli - @billy-bao
  • [x] Create lff package and create lff application that applies formatter - @billy-bao
  • [x] Factor out WIP toward new FedGenerator (to be submitted in a separate PR)

petervdonovan avatar Jun 13 '22 08:06 petervdonovan

Do you think it would be possible to combine this with target language formaters and run, let's say, clang-format on the code bodies?

Yes, I believe we plan to do that, although it may be a separate PR.

petervdonovan avatar Jun 20 '22 16:06 petervdonovan

I just realized that this approach to formatting can make code like this unreadable:

    test = new TraceTesting(
        events_size = 5,
        trace_size = 165,
        trace = (
            0,0,0,1,1,0,0,0,0,0,0,
            500000000,0,0,0,1,1,1,0,0,0,0,
            250000000,0,0,1,1,0,1,0,0,0,0,
            250000000,1,1,0,1,0,1,0,0,0,0,
            0,0,1,0,1,0,1,1,1,0,0,
            500000000,0,1,0,1,0,1,0,1,1,1,
            250000000,0,1,0,1,0,1,1,1,0,1,
            250000000,1,1,0,1,0,1,0,1,0,1,
            250000000,0,1,0,1,1,1,0,1,0,1,
            250000000,0,1,1,1,0,1,0,1,0,1,
            500000000,1,1,0,1,1,1,0,1,0,1,
            0,0,1,0,1,0,1,1,1,0,1,
            500000000,0,1,0,1,0,1,0,1,1,1,
            250000000,0,1,0,1,0,1,1,1,0,1,
            250000000,1,1,0,1,0,1,0,1,0,1
        ), training = false)

The placement of newlines in this trace definition seems important, but if we are generating straight from the AST, that information is erased. It ends up looking like:

    test = new TraceTesting(
        events_size = 5,
        trace_size = 165,
        trace = (
            0,
            0,
            0,
            1,
            1,
            0,
            0,
            0,
            0,
            0,
            0,
            500000000,
            0,
            0,
            0,
            ...

My basic idea here was that the goal of the programmer is to craft an AST, not a text file (programs != ASCII art), but this is an exception.

We have the option to let the programmer include a comment to force the original format to be preserved. Something like:

        trace = (  // keep-format
            0,0,0,1,1,0,0,0,0,0,0,
            500000000,0,0,0,1,1,1,0,0,0,0,
            250000000,0,0,1,1,0,1,0,0,0,0,
            ...

This probably would be easy to implement, but it is not very pretty. I'm open to other ideas.

petervdonovan avatar Jul 04 '22 08:07 petervdonovan

I don't quite understand the code example. Is this LF code or target code?

Also other code formatters face the problem that you describe. Special comments that switch of the formatting are the standard solution. When it comes to formatting, there is also a lot of opinion involved about the right or best style. We could also consider making certain decisions configurable, but this is probably something we would tackle further down the road. However, personally, I like the philosophy of the Python formatter black, which does not allow configuration.

cmnrd avatar Jul 04 '22 08:07 cmnrd

I don't quite understand the code example. Is this LF code or target code?

It is LF code, copied directly from a modal models test.

Yes, I also like Black and was inspired by it. I do not really want configurability. I just brought up this issue because a near-term goal is to format all of our tests with this formatter, and because I think even a Black fanatic would agree that the lists of numbers that appear in the modal models tests do benefit greatly from their particular use of whitespace.

petervdonovan avatar Jul 04 '22 09:07 petervdonovan

Ok, I see. I would be absolutely in favour of adding a comment that can switch formatting off. Black uses this:

# fmt: off
np.array(
    [
        [1, 0, 0, 0],
        [0, -1, 0, 0],
        [0, 0, 1, 0],
        [0, 0, 0, -1],
    ]
)
# fmt: on

The main problem that I see, is that we need to be somehow able to switch formatting back on or limit the scope. In your proposed solution:

        trace = (  // keep-format
            0,0,0,1,1,0,0,0,0,0,0,
            500000000,0,0,0,1,1,1,0,0,0,0,
            250000000,0,0,1,1,0,1,0,0,0,0,
            ...

would the // keep-format only apply to the annotated AST node?

We could also consider using the new annotations for this purpose. This might be clearer than comments, espacially since it would clearly separate annotations from comments in target code blocks that influence target code formatting.

cmnrd avatar Jul 05 '22 07:07 cmnrd

would the // keep-format only apply to the annotated AST node?

Yes. As it turns out, having a single comment that applies to the annotated AST node is much easier to implement than using two comments to toggle the formatter on and off. This is because, in order for the formatter to work correctly, it has to associate comments with AST nodes regardless. I would like to keep the // keep-format approach unless anyone feels sure that a different approach is better.

petervdonovan avatar Jul 09 '22 01:07 petervdonovan

I think force-pushing to this branch that has dependents is not a great idea. This created a lot of issues in the fed-gen branch because the history was lost.

Soroosh129 avatar Jul 28 '22 10:07 Soroosh129

Can we try to merge this soon?

I promise that the change I just made was trivial and that this has mostly just been awaiting review for 23 days.

petervdonovan avatar Aug 29 '22 23:08 petervdonovan

Sorry, this was not on my radar as my review wasn't requested. I'll have a look at this soon. I agree, we should get this merged...

lhstrh avatar Aug 30 '22 05:08 lhstrh