diku-dk / futhark

:boom::computer::boom: A data-parallel functional programming language
http://futhark-lang.org
ISC License
2.41k stars 166 forks source link

Futhark Formatter #2178

Closed WilliamDue closed 1 month ago

WilliamDue commented 2 months ago

This pull request contains the work in progress code formatter for Futhark.

athas commented 2 months ago

If you want submodules, then put them under Futhark.Fmt. There is one rule for the Futhark.CLI hierarchy, and that is that the modules there corrspond exactly to CLI executables.

athas commented 1 month ago

I have pushed a change to Futhark.CLI.Fmt that makes it read input from stdin if no file is given.

WilliamDue commented 1 month ago

The Futhark.CLI.Fmt.AST module does not, in fact, define an AST.

Although you should consider the formatting of the following file.

def x = 0x123

You will notice that the output is wrong. The solution is to keep around the original source bytestring and use the offsets in the AST location information to retrieve the original syntax for number literals.

We should have solved this problem here. Also we will think of a better name for AST.

athas commented 1 month ago

Since this is now pretty much functionally correct, I have marked it for review. Fix all the style issues please.

WilliamDue commented 1 month ago

We are decently happy with this version. Currently trailing comments works but it does not work perfectly see tests/fmt/trailingComments0.fut. We think we could do a hacky solution but it seems like we would have to maybe rethink some design choices a bit to make it work in a none hacky way.

athas commented 1 month ago

What are all those Zone.Identifier files?

WilliamDue commented 1 month ago

In technical terms, we have made a whoopsie daisy.

WilliamDue commented 1 month ago

Please don't reformat files that are not relevant to your work.

I think the problem is gone now.

athas commented 1 month ago

tests_fmt/expected would be more interesting if all the input files were not already formatted.

athas commented 1 month ago

Can I merge this now?

thereseLyngby commented 1 month ago

Test script in tests_fmt is not done yet, but should be in next hour or so. The test themselves remain "uninteresting", most of them cover issues, where the AST used to represent two different code snippets with the same term (like the pattern thing in loops).

athas commented 1 month ago

So it is done now?

WilliamDue commented 1 month ago

We have added it to CI now it should hopefully be "done" now. I think we have some edge case problems with correctly place comments dealing with sep but they should probably be squashed as they come. One of these are sumtypes, they are not located if they do not take an argument.

WilliamDue commented 1 month ago

We think ideally the Fmt type should also be Locatedsuch that comments can be placed correctly and the location should be based on the expression that is formatted. And when concatenated with another Fmt the smallest location that is not NoLoc should be used for the new Fmt.

athas commented 1 month ago

chmod +x tools/testformatter.sh

WilliamDue commented 1 month ago

Actually a side note, there is probably quite alot of cases where sep places comments wrongly. This is because we had to change how sep worked because we wanted trailing comments. The thing with Fmt having a locaiton is probably not that good actually.