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

fmt: defaults produce different results between two versions #6726

Closed
2 tasks done
riordant opened this issue Jan 8, 2024 · 3 comments
Closed
2 tasks done

fmt: defaults produce different results between two versions #6726

riordant opened this issue Jan 8, 2024 · 3 comments
Labels
T-bug Type: bug

Comments

@riordant
Copy link

riordant commented Jan 8, 2024

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (02292f2 2024-01-08T00:22:29.377806466Z)

What command(s) is the bug in?

forge fmt

Operating System

Linux

Describe the bug

Hi guys,

we have a strange issue with formatting, that was only introduced from the nightly-5b7e4cb3c882b28f3c32ba580de27ce7381f415a release (2023-12-02) onwards. it appears that the default formatting rules have changed.

here is how to repro:
clone https://github.com/spoolfi/spool-v2-core/, forge install

foundryup -v nightly-60ec00296f00754bc21ed68fd05ab6b54b50e024 (version 2023-11-01)
forge fmt - produces no formatting changes.

foundryup -v nightly-5b7e4cb3c882b28f3c32ba580de27ce7381f415a (version 2023-12-02)
forge fmt - produces a host of changes.

Our toml file has no [fmt] entries indicating that it should be using the defaults.

This means that we can no longer update foundry from this version without the source being modified.

i have tried to figure out exactly where the issue was introduced, but the only formatting changes are to introduce hex_underscore rules which should not affect it.

@riordant riordant added the T-bug Type: bug label Jan 8, 2024
@gakonst gakonst added this to Foundry Jan 8, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Jan 8, 2024
@mattsse
Copy link
Member

mattsse commented Jan 8, 2024

I see a few fixes in the diff, like enum comments, uint256 over uint, but most diffs are things like

-        curveStrategy = new Curve3poolStrategyHarness(
-            assetGroupRegistry,
-            accessControl,
-            assetGroupId,
-            swapper
-        );
+        curveStrategy = new Curve3poolStrategyHarness(assetGroupRegistry, accessControl, assetGroupId, swapper);

or

-            StrategyRegistry implementation = new StrategyRegistry(
-                masterWallet,
-                spoolAccessControl,
-                usdPriceFeedManager,
-                address(ghostStrategy)
-            );
+            StrategyRegistry implementation =
+                new StrategyRegistry(masterWallet, spoolAccessControl, usdPriceFeedManager, address(ghostStrategy));

which would point to #6408 which I consider a fix because this properly added new stmt formatting, but perhaps we need to tune this.

any decision you disagree with in the diff?

ermyas added a commit to immutable/zkevm-bridge-contracts that referenced this issue Jan 8, 2024
The most recent version of Foundry seems to have an open bug. This bug causes the formatter check to produce a different result compared to previous versions. This commit reverts to the version of Foundry that was used before the bug was introduced.
foundry-rs/foundry#6726
@riordant
Copy link
Author

riordant commented Jan 9, 2024

Ok, thanks for pointing it out. I was just trying to figure out if this was a breaking change or a bug, looks like the former then.

No particular thing we disagree with, it's just that it modifies audited code in our src/, which we do not want to change anymore.

Is there anyway to preserve the previous new rules with new versions? I guess not as there is not a format option for new expressions?

@DaniPopes
Copy link
Member

Is there anyway to preserve the previous new rules with new versions? I guess not as there is not a format option for new expressions?

No, this was a bug fix because new expressions were getting skipped altogheter.

it's just that it modifies audited code in our src/, which we do not want to change anymore.

You should ignore the directory instead of relying on us not to ever change the formatter

@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

3 participants