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

Fix: Formatting within the IDE wasn't working anymore #4274

Merged
merged 18 commits into from
Jul 14, 2023

Conversation

MikaelMayer
Copy link
Member

This PR fixes #4269 and fixes #4270
I first added unit tests for the cases that were failing. What really solved the issue was that I had to implement proper cloning for

  • methods, that were not keeping BodyStartTok
  • formals, that were not keeping RangeToken

Then I also added for all formatting test a case when it tries to clone all members before formatting, like VSCode does, which unveiled 3 more formatting issues:

method Test() {
  g := new ClassName.ConstructorName(
      argument1b, // Two extra spaces
      argument2b,
      argument3b
    );
  var g := new ClassName.ConstructorName(
           argument1, // Missing two extra spaces (when block mode activated)
           argument2,
           argument3
         );
...
  && unchanged(
       this,
       c
     )
     && old( // Extra undesired space here
         c.c
       ) == c.c
  && fresh(
       c.c
     )

I also solved the above issues by:

  • Ensuring the RangeToken of TypeRHS is preserved after cloning by modifying its topmost cloning parent AssignmentRHS
  • Ensure we iterate on pre resolved children to get OwnedToken (otherwise some cloned expressions were crashing)
  • Ensure UnchangedExpr is cloned with the same pattern as everyone else (otherwise the RangeToken wasn't kept)

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

docs/dev/news/4269.fix Outdated Show resolved Hide resolved
@MikaelMayer MikaelMayer self-assigned this Jul 11, 2023
@@ -323,6 +323,8 @@ public enum AccumulationOperand { None, Left, Right }
if (cloner.CloneResolvedFields) {
ResolvedOp = original.ResolvedOp;
}

RangeToken = original.rangeToken;
Copy link
Member

Choose a reason for hiding this comment

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

The call to base already does RangeToken = cloner.Range(original.RangeToken);, so this line seems obsolete

@@ -101,6 +102,22 @@ public class Method : MemberDecl, TypeParameter.ParentType, IMethodCodeContext,
Contract.Invariant(Decreases != null);
}

public Method(Cloner cloner, Method m) : base(cloner, m) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename m to original?

Copy link
Member

@keyboardDrummer keyboardDrummer left a comment

Choose a reason for hiding this comment

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

Looks great, just one request for change :)

@MikaelMayer MikaelMayer enabled auto-merge (squash) July 12, 2023 16:31
@MikaelMayer MikaelMayer merged commit d8c1135 into master Jul 14, 2023
19 checks passed
@MikaelMayer MikaelMayer deleted the fix-4269-format-lemma-ide branch July 14, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format parameters the IDE Format lemma in the IDE
2 participants