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

Forbid read-eval syntax in formatted/linted files #180

Closed
wants to merge 1 commit into from

Conversation

vemv
Copy link
Contributor

@vemv vemv commented Mar 17, 2021

Brief

Forbid read-eval syntax in formatted/linted files

This particularly targets Eastwood (since it's the only member of our stack that evals code, and therefore can run read-eval syntax) and tools.reader (which we internally use for various purposes).

It also is bound at formatting-stack.core level in case any other mechanisms happen to be sensitive.

Fixes #158

QA plan

  • Lint formatting-stack itself via the project or branch formatter
    • Note that Eastwood will complain due to the two test namespaces that intentionally fail (functional.formatting-stack.linters.eastwood.examples.read-eval, functional.formatting-stack.linters.eastwood.examples.read-eval-2)
    • Note that they must be placed in test and not test-resources, since ns parsing (and refresh-dirs parsing) is part of how our Eastwood integration works
    • This can be slightly inconvenient in a future when we dogood f-s in CI (not done atm), by then we could simply exclude these two files with a strategy.

Author checklist

  • I have QAed the functionality
  • The PR has a reasonably reviewable size and a meaningful commit history
  • I have run the branch formatter and observed no new/significative warnings
  • The build passes
  • I have self-reviewed the PR prior to assignment
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:

Reviewer checklist

  • I have checked out this branch and reviewed it locally, running it
  • I have QAed the functionality
  • I have reviewed the PR
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

This particularly targets Eastwood (since it's the only member of our stack that evals code, and therefore can run read-eval syntax)
and tools.reader (which we internally use for various purposes).

It also is bound at `formatting-stack.core` level in case any other mechanisms happen to be sensitive.

Fixes nedap#158
@vemv
Copy link
Contributor Author

vemv commented Mar 17, 2021

Not ready yet:

defecord constructor syntax (#formatting_stack.strategies.foo.Foo[:a]) inherently uses read-eval. This alone shouldn't prevent a file from being formatted/linted.

Instead one could detect if there's #= in a given file (using e.g. rewrite-clj), and bind *read-eval* accordingly.

@vemv
Copy link
Contributor Author

vemv commented Sep 17, 2021

Personally I'm not that interested in forbidding read-eval anymore. Or at least under a direct approach. It just gives too many problems IME - read-eval is sprinkled throughout Clojure internals so disabling it can be problematic.

Another approach would be linting this 'syntactically' i.e. with rewrite-clj.

@vemv vemv closed this Sep 17, 2021
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.

f-s must be run binding *read-eval* to false
1 participant