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

Throw error if code formatting ever changes semantics #3194

Open
OAGr opened this issue Apr 18, 2024 · 0 comments · May be fixed by #3195
Open

Throw error if code formatting ever changes semantics #3194

OAGr opened this issue Apr 18, 2024 · 0 comments · May be fixed by #3195
Labels
Error Messages An error message is not descriptive enough Language Regarding Squiggle language semantics, distributions and function registry Web components

Comments

@OAGr
Copy link
Contributor

OAGr commented Apr 18, 2024

  • Is new feature

Description of suggestion or shortcoming:

We've had a few code formatting bugs before. These can be really annoying for users, as they can be difficult to detect and debug (they often don't leave errors, just change code).

It seems good to check the AST before+after, and make sure these are identical semantically. If they aren't, throw an error, instead of formatting.

I started trying this.

In useFormatSIquiggleExtension.ts

export async function formatSquiggle(view: EditorView | undefined) {
  if (!view) return;
  const code = view.state.doc.toString();
  const before = JSON.stringify(parse(code));
  const { formatted, cursorOffset } = await prettier.formatWithCursor(code, {
    parser: "squiggle",
    plugins: [prettierSquigglePlugin],
    cursorOffset: view.state.selection.main.to,
  });
  const after = JSON.stringify(parse(formatted));
  console.log("AST", before === after);
  console.log(before);
  console.log(after);
  view.dispatch({
    changes: {
      from: 0,
      to: view.state.doc.length,
      insert: formatted,
    },
    selection: {
      anchor: cursorOffset,
    },
  });
}

Here, I found that the AST does change based on the location objects, which makes sense, even though these don't have semantic meaning. So we'd need to detect changes other than location changes.

One easy way to do this is to write a single-use script that converts the data into JSON, then scrubs the locations, then checks for equality.

A better way would probably be something like adding parameterized isEqual methods for ASTNode.

@OAGr OAGr added Language Regarding Squiggle language semantics, distributions and function registry Web components Error Messages An error message is not descriptive enough labels Apr 18, 2024
@github-project-automation github-project-automation bot moved this to 🆕 To prioritize in Main project Apr 18, 2024
@OAGr OAGr linked a pull request Apr 18, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Messages An error message is not descriptive enough Language Regarding Squiggle language semantics, distributions and function registry Web components
Projects
Status: 🆕 To prioritize
Development

Successfully merging a pull request may close this issue.

1 participant