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

feat: make loose docstrings into elaboration errors rather than parse errors #4975

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kmill
Copy link
Collaborator

@kmill kmill commented Aug 9, 2024

Now loose docstrings will not yield a confusing parser error and instead log an "unexpected docstring" elaboration error and continue processing.

Furthermore, if the docstring is preceding the in command combinator, it will add a hint to the error:

unexpected doc string

Hint: the docstring must come after '... in', immediately before the declaration that accepts a docstring.

Closes #3135

@kmill kmill requested a review from Kha as a code owner August 9, 2024 17:25
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Aug 9, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Aug 9, 2024

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 30a52b794a39256361050c52fe0e41bbff91bc8d --onto dd6ed124baef64a26ef5860f49597fdcef371239. (2024-08-09 17:43:40)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 30a52b794a39256361050c52fe0e41bbff91bc8d --onto 5f31e938c1bf5bb2d6d2d29b26ea932ade115357. (2024-08-12 18:18:41)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase e5e577865fe74ad17182346a225f8f209fa6e2e4 --onto 3ec55d3d498ce361f5886afee516d16df5cbd6fc. (2024-09-07 21:36:32)

@Kha
Copy link
Member

Kha commented Aug 13, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit f1bb524.
There were significant changes against commit 30a52b7:

  Benchmark   Metric                 Change
  ====================================================
- parser      instructions             6.3% (6414.6 σ)
+ stdlib      fix level params        -1.8%  (-13.9 σ)
- stdlib      instantiate metavars     1.6%   (38.9 σ)

… errors

Now loose docstrings will not yield a confusing parser error and instead log an "unexpected docstring" elaboration error and continue processing.

Furthermore, if the docstring is preceding the `in` command combinator, it will add a hint to the error:
```
unexpected doc string

Hint: the docstring must come after '... in', immediately before the declaration that accepts a docstring.
```

Closes leanprover#3135
@kmill kmill force-pushed the loose_doc_comment branch from f1bb524 to bd4c12d Compare September 7, 2024 21:19
@kmill
Copy link
Collaborator Author

kmill commented Sep 7, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit bd4c12d.
There were significant changes against commit e5e5778:

  Benchmark       Metric          Change
  ==================================================
- bv_decide_mul   branch-misses     2.6%    (13.5 σ)
- import Lean     task-clock       14.1%    (22.2 σ)
- import Lean     wall-clock       14.1%    (22.2 σ)
- parser          instructions      6.3% (15429.4 σ)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nicer error message if set_option goes after doc string
4 participants