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

VS Code: Validate generated code #828

Merged
merged 110 commits into from
Jan 18, 2022
Merged

Conversation

petervdonovan
Copy link
Collaborator

@petervdonovan petervdonovan commented Jan 7, 2022

This PR adds validation on save, with error/warning/info/hint squiggles (usually) reported to the correct lines. In master, this feature only exists for C++.

Changes:

  • C++
    • MSVC: Now syntax checks can be performed on Windows on save, without compiling.
    • Known issues:
      • Clang-tidy is too slow to be the default syntax checker on any OS. Clang should later be added has been added as an alternative.
      • MSVC checks syntax slowly.
  • Python
    • Pylint is now the default Python linter. It is used if it is installed.
    • compileall is used for syntax checking as a backup.
  • TypeScript
    • ESLint is used for linting. It can always be used (by anyone with npm/pnpm) because it is a dev dependency in package.json.
    • tsc is used in addition to ESLint for slower, more thorough checks, but only if ESLint does not find any errors.
  • Rust
    • Clippy is the Rust linter. It can always be used (by anyone with cargo) because it comes with cargo.

Changes will be added for C when that part of the code base stabilizes. Validation on save currently does not work properly for the C target.

petervdonovan and others added 30 commits December 28, 2021 21:13
(and refactor for code reuse)
This was mentioned during code review. IntelliJ automates this, so this change should make the guideline easier to follow.
I assume that people will be unhappy if compilation fails just because they used `let` instead of `const`.
This change does not affect standalone mode or complete builds.
This commit addresses a weird special case that we should not support (#657). We should revert it.
This is necessary to keep changes in this branch from making TSGenerator harder to maintain. It also addresses a TODO that has been at the top of doGenerate for some time.
I assume that these bugs were tolerated or ignored because they are in error reporting code, which means they only cause errors when some other error has already occurred.
Resolves conflicts in TSGenerator.kt.
@petervdonovan petervdonovan force-pushed the vscode-validate-generated-code branch 6 times, most recently from 27a6386 to b63c234 Compare January 17, 2022 01:58
This tests handling of cases in which preferred software is not present on the system.
@petervdonovan petervdonovan force-pushed the vscode-validate-generated-code branch from b63c234 to c17701b Compare January 17, 2022 03:58
@petervdonovan
Copy link
Collaborator Author

petervdonovan commented Jan 17, 2022

Hm, the only difference between the latest commit and c17701b (for which all tests passed, except benchmark tests which were canceled to save resources) is an extra CodeCov report, and now ReflexGameTest seems to fail in a way that I cannot reproduce locally on the same OS. This seems very strange.

Edit: (probably) solved by 49806b3, but still with many unanswered questions.

It is a complete mystery to me why this bug appeared when it did (after a clearly unrelated change), why it did not appear earlier, and why I could not reproduce it on my machine.
@petervdonovan
Copy link
Collaborator Author

I am very glad @lhstrh encouraged me to write more tests in this branch because the tests uncovered several bugs.

#850 should probably be merged before this PR. Other than that, I think this is PR is finally ready, unless there are any last-minute concerns, or unless we would like to merge scalability-banks first so that the conflicts are less bad.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

I agree, this looks ready to merge. This represents a really significant chunk of work that's really crosscutting. Truly impressive!

.github/workflows/lsp-tests.yml Show resolved Hide resolved
example/C/src/ReflexGame/ReflexGameTest.lf Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/ASTUtils.xtend Outdated Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Jan 18, 2022

Note that CI is not running because of a misconfiguration: https://github.com/lf-lang/lingua-franca/actions/runs/1711584881

@petervdonovan
Copy link
Collaborator Author

Right, this is because workflow references have to be changed to master in order for this branch to be deleted. I will revert that change to run CI, but then revert the revert. The inconsistency should be taken care of by the merge.

@petervdonovan petervdonovan merged commit 1da8d58 into master Jan 18, 2022
@petervdonovan petervdonovan deleted the vscode-validate-generated-code branch January 18, 2022 16:07
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.

3 participants