feat: fmt config

[rkrasiuk]

Component

Forge

Describe the feature you would like

The formatter uses a configuration with hardcoded values. The goal is to define the input format for the configuration (i.e. whether the configuration will be defined in a separate file like fmt.toml or it will become a part of foundry config) and, potentially, expand the options.

Currently, the supported configuration values are:

• line_length - maximum line length where formatter will try to wrap the line
• tab_width - number of spaces per indentation level
• bracket_spacing - print spaces between brackets

Additional context

No response

[shekhirin]

I like the idea of having it inside foundry.toml under fmt/format/formatter section to not spread the configuration of the whole toolkit over several files. I like how TOML allows you to have distinct sections not messing up the whole look of config. Also it differs from rustfmt which has its own rustfmt.toml in the way that it's impossible to have forge fmt without other CLIs and commands.

Regarding the options, I think at least single_quote: bool (or quote_style: single|double) and explicit_types: bool are missing, I feel like they shouldn't be opinionated by us and non-configurable.

[gakonst]

Agree on foundry.toml under fmt section (default: empty).

CC @transmissions11 @mds1 @lucas-manuel @FrankieIsLost for more opinions on config options.

[transmissions11]

these options lgtm rn, tho as i use it more i will probably come up with new ones

[mds1]

Looking at the above + prettier-plugin-solidity, here is what I think the full list of options can be

• line-length
• tab-width
• bracket-spacing
• single-quote / quote-style
• explicit-types

Agree on foundry.toml under fmt section (default: empty).

What does this look like exactly, i.e. is the fmt section a different header style than profiles? Or do you just have a rule that a header name of [fmt] is not a profile but otherwise it looks the same as a profile header?

[gakonst]

Good q - how should we be thinking about it @mattsse? I think having a [fmt] which looks like a profile but is not a profile could be fine, maybe we should do it like [[fmt]]? open to ideas

[mattsse]

Having it directly in foundry.toml makes sense, although this binds forge fmt to the foundry toolchain, but having it in foundry.toml is the best solution because you can configure paths directly there, so it can be configured for the project layouts (custom src path etc..) of other tools (hardhat).

a [fmt] section should be fine, doubt we run into issues there.

this will cover project specific settings.

for global settings, we should do the same for ~/.foundry/foundry.toml, right?

[onbjerg]

A bit offtopic: I've talked a bit about this before but I think we should at least consider moving towards a more structured config format, especially if we at some point want to support different VMs and/or languages. Essentially something a bit more like Cargo where profiles would be e.g. [profile.<name>], solc config would be under [solc] etc.

[gakonst]

this binds forge fmt to the foundry toolchain

we call that a moat sir, jk. we can make forge fmt take CLI args / env vars instead of just foundry toml.

for global settings, we should do the same for ~/.foundry/foundry.toml, right?

yes - although i think we dont use a global foundry toml anywhere yet?

Essentially something a bit more like Cargo where profiles would be e.g. [profile.], solc config would be under [solc] etc.

agreed, I like the idea of adding a profile prefix to anything that's a profile.

[mds1]

+1 on namespacing things in the config file like [profile.<name>], I like that much better than just inferring that [fmt] is not a profile name. Only issue is that'd be a breaking change right? Unless for some amount of time both are support, and log a big warning if the old format is detected

[gakonst]

personally feel OK with a small breaking change like this, but good reminder for us to start specifying stabilization criteria (milestone probably being fmt/coverage/multichain forking/invariant tests/fuzzer improvements/stable configs?).

also OK with a warning, as long as it doesn't introduce a bunch of code churn to detect the old format.

[onbjerg]

Imo as we're moving towards a stabler release we have some breaking changes backlogged that we could roll out in "one go" since foundryup would most likely break at the same time after RIIR.

Depending on feasibility we could even tag a "pre-stable" version before we cut our first stable that people could use until they get around to migrating depending on how much breakage there is

[shekhirin]

since foundryup would most likely break at the same time after RIIR

wouldn't it be possible to seamlessly replace bash version with the new Rust one after foundryup of newer toolkit version?

[onbjerg]

On some platforms maybe, but overwriting currently running programs have odd edge cases on some platforms :/

[sambacha]

Looking at the above + prettier-plugin-solidity, here is what I think the full list of options can be

• line-length

• tab-width

• bracket-spacing

• single-quote / quote-style

• explicit-types

Agree on foundry.toml under fmt section (default: empty).

What does this look like exactly, i.e. is the fmt section a different header style than profiles? Or do you just have a rule that a header name of [fmt] is not a profile but otherwise it looks the same as a profile header?

You are missing by compiler version, prettier solidity lets you adjust depending on the pragma version.

Additionally you can sort imports as well, here us an example:

importOrder: ["^@forge-std/(.*)$", "^ds-test/(.*)$", "^@openzeppelin/(.*)$", "^~(.*)$", "^[./]"],
  importOrderSeparation: true,
  importOrderSortSpecifiers: true,

line-length isn't an option, printWidth is, see the discussion here: prettier-solidity/prettier-plugin-solidity#474 (comment)

This option is misleading as its not a count of characters per line (which makes sense as it would break code if it strictly enforced that).

Also probably the most requested feature is // prettier-ignore

If you can add maybe something like
/*! forge-fmt-disable */
/*! forge-fmt-enable */

Would be 💯🚀🍻

[mds1]

Ah specifying import sort order would be great 👌

[sambacha]

Ah specifying import sort order would be great 👌
just missed my updated comment wdyt?

[jpopesculian]

I wanted to clarify, the foundry.toml changes before I go ahead and implement this. Do we want to still have a [default] section and then the [profile.<name>] would override this section, and we just have one [fmt] section? Or is it more like we change the [default] section to [forge] (or split it up into [build] / [test] etc) and have be able to override via [profile.<name>.forge] (or [profile.<name>.build] or test or whatever) and allow for profile based fmt options via [profile.<name>.fmt]?

[mds1]

My current thinking is something like: [profile.default], [profile.ci], etc. for the main profiles, which contain everything that affects the build/test/deploy process. Then we'd have "special" sections that are independent of that, where profiles don't really have much value, like [fmt] and eventually [doc] and [lint] as other future examples.

I think having separate [build] and [test] sections/profiles might overcomplicate things a bit for minimal benefit, since often times the two are coupled (e.g. no solc optimizer and small fuzz runs for local tests, optimizer + high runs in CI). However, I could likely be convinced that approach is valuable, since it'd mean the config settings basically each map to a forge <cmd>. Main downside is then you need to specify multiple profiles to run forge test which is a little clunky

[gakonst]

My current thinking is something like: [profile.default], [profile.ci], etc. for the main profiles, which contain everything that affects the build/test/deploy process. Then we'd have "special" sections that are independent of that, where profiles don't really have much value, like [fmt] and eventually [doc] and [lint] as other future examples.

I think we should go with that, the alternative seems to be more complicated than we want rn. So let's go with the former @jpopesculian

[jpopesculian]

If you want, i can also just parse all unknown groups as profiles and output a deprecation warning in order to break as little as possible

[mattsse]

My current thinking is something like: [profile.default], [profile.ci], etc. for the main profiles, which contain everything that affects the build/test/deploy process. Then we'd have "special" sections that are independent of that, where profiles don't really have much value, like [fmt] and eventually [doc] and [lint] as other future examples.

I think we should go with that, the alternative seems to be more complicated than we want rn. So let's go with the former @jpopesculian

I like this as well, this will free us up to do a [profile.solc] section as well, so we can finally improve various solc settings for multiple compilers

[gas1cent]

Would be great to replace non-checksummed addresses with checksummed.
And some indication that the formatter is working or has just finished would be nice. Might be a progress bar or a summary of changes. Took me 3 runs to realize that it's not broken, just takes 0.3 seconds and doesn't produce any output.

[mattsse]

do we have a linting wishlist, of tracking issue for forge --fix @rkrasiuk ?

[rkrasiuk]

@mattsse not to my knowledge

[sambacha]

Would be great to replace non-checksummed addresses with checksummed. And some indication that the formatter is working or has just finished would be nice. Might be a progress bar or a summary of changes. Took me 3 runs to realize that it's not broken, just takes 0.3 seconds and doesn't produce any output.

cast already has an option for providing a checksummed address.

do we have a linting wishlist, of tracking issue for forge --fix @rkrasiuk ?

What is forge --fix suppose to fix? Isn't fmt suppose to be like prettier? Prettier is not a linter, the distinction is important, which is why I ask.

[0xGorilla]

Is anyone actively working on this? I would like to help a bit if possible

[rkrasiuk]

@0xGorilla, Julian (@jpopesculian) is tackling this one

[gakonst]

This is in master now, [fmt] can be configed as described at the OP. I don't think we need to track forge fix yet because we don't have/have designed it

[fvictorio]

For the record, we are going to remove the explicitTypes option from prettier-solidity, since we think that's something that should be done by a linter, not a formatter. For the same reason, I would recommend not including an option for sorting the imports, but YMMV.

[mds1]

since we think that's something that should be done by a linter, not a formatter. For the same reason, I would recommend not including an option for sorting the imports

Oh interesting, can you expand on why? The types don't affect bytecode at all so it feels like a formatting/style choice only. (I'm not sure if this is also true for import order). Checked the PR (prettier-solidity/prettier-plugin-solidity#730) and couldn't find any discussion around the decision/controversy, though I did see it affects the AST but from a user-standpoint they won't notice that which makes it feel like just a style preference

[fvictorio]

It's not a black-and-white thing, admittedly. To me, it always felt wrong, and some people have told me they find it a weird thing for prettier-solidity to do. Plus, I'm very much in favor of having as few options in the formatter as possible.

The thing that finally convinced me is realizing that prettier-core doesn't re-write Array<string> as string[] (or the other way around), even when they are semantically equivalent. If you want to enforce a style for that, you would use eslint.