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

Version 3.0 #408

Merged
merged 53 commits into from
May 15, 2021
Merged

Version 3.0 #408

merged 53 commits into from
May 15, 2021

Conversation

pfoerster
Copy link
Member

@pfoerster pfoerster commented May 4, 2021

This PR consists of a huge internal refactoring and a lot of bug fixes. As mentioned in #405, there are some breaking changes concerning mainly the options format and additional requests like building and forward search.

Closes #405.
Closes #365.
Closes #359.
Closes #339.
Closes #409.
Closes #386.
Closes #310.
Closes #413.
Closes #309.

Checklist for 3.0:

  • Reimplement ChkTeX and try to provide a fix for Spwaning tons of chktex #186.
  • Regression: Hover over labels not working
  • Regression: Completion sometimes calculates a wrong TextEdit
  • Regression: Parent expansion when analyzing the workspace is too greedy
  • Regression: Find all references performance
  • Static Analysis Bug: False positives, especially when defining environments

@pfoerster
Copy link
Member Author

@clason The hover issue should be fixed now. The stress test project was very helpful 👍

@pfoerster
Copy link
Member Author

Looking at the stress test project, it looks like there are some false positives concerning the static analysis as well. Those should be fixed too.

@clason
Copy link
Contributor

clason commented May 4, 2021

Glad it's useful for something :) Can confirm that hover works here as well -- thanks!

Yes, I noticed the false positives (or negatives?) as well, but put them on the back burner as merely cosmetic.

I still see some, though; for example:

\newenvironment{shortexample}{%
      \begingroup\mdfsetup{nobreak=true}
      \begin{example}%
E }{%     ■ Unexpected "}"
E     \end{example}\endgroup%     ■ Missing "}" inserted
  }

(with the static analysis error inline). There are also some in inline asymptote code, but that's a different matter.

@pfoerster
Copy link
Member Author

@clason The workspace expansion issue should be fixed along with the "find all references" performance regression. Concerning the static analysis, I managed to reduce the number of false positives a bit. Most of them are now inside the asymptote code. In particular, texlab now understands the \newenvironment command.

@pfoerster
Copy link
Member Author

@clason Did the completion regression with the wrong text edit occur in the mean time again or can it be marked as resolved? Besides that, I decided to reimplement building and forward search because some people likely rely on these features and I feel bad removing them without an alternative. All in all, I think this PR can be merged soon.

@clason
Copy link
Contributor

clason commented May 14, 2021

I've been using this PR as my daily driver for a while and have not noticed this problem since then, so I'd consider it fixed. (And if not, it's random enough to merge anyway and I'll open an issue should it occur again.)

The only thing that I've noticed in my testing is some differences in the (workspace) symbols, especially the styling. For example (from a newer version of the above arXiv project; left is master, right is this PR)

functan.tex|19 col 9| [item] (i)    ▎ functan.tex|19 col 9| [item] i

(formatted item labels are always missing (...)) or

 functan.tex|381 col 1| [thm] Theorem 1.9 (Eberlein--\u{S}mulyan)    ▎ functan.tex|381 col 1| [thm] Theorem 1.9

(theorem names are missing if they involve a LaTeX command) or

graphical.tex|16 col 1| [sec] 22.1 Semi-differentiability    ▎ graphical.tex|16 col 1| [sec] Semi-differentiability

(some section numbers are missing). There's also some whitespace difference in float captions (I believe an added space before and after the caption? The space before the caption is not consistent, though.)

For some reason, the symbols for float break my client showing them (neovim with telescope.nvim) with this PR; not sure why that should be the case if they worked before...

@clason
Copy link
Contributor

clason commented May 14, 2021

Oh, and I'm no longer seeing diagnostics from the .log file? (Parser diagnostics still show up.)

Not that I miss them, exactly :p

@pfoerster
Copy link
Member Author

@clason The issues with the symbol formatting should be fixed now.

Oh, and I'm no longer seeing diagnostics from the .log file? (Parser diagnostics still show up.)

This was some regression caused by incorrect debouncing. The parsing events caused the build log diagnostics to be debounced but now it should be fixed.

Not that I miss them, exactly :p

Almost nobody misses overfull hboxes ;)

@clason
Copy link
Contributor

clason commented May 14, 2021

@clason The issues with the symbol formatting should be fixed now.

Not completely; some symbols are still not showing the numbers -- but now it's different ones, so maybe a timing issue? (The other issues are indeed fixed.)

This was some regression caused by incorrect debouncing. The parsing events caused the build log diagnostics to be debounced but now it should be fixed.

👍🏻

Almost nobody misses overfull hboxes ;)

Indeed :)

@pfoerster
Copy link
Member Author

Not completely; some symbols are still not showing the numbers -- but now it's different ones, so maybe a timing issue? (The other issues are indeed fixed.)

The label numbers are calculated synchronously so I don't think its a timing issue. Does it change over time? Can you take a look at the .aux file and see if the labels are defined there? That's the first possibility, namely that texlab cannot parse \newlabel command corrrectly. Another thing would be that it cannot find the label at all but then you would not see a label name. That's a bit odd because I cannot reproduce the issue on my machine. In particular, I verified that every label in your sample project has a number associated with it.

@clason
Copy link
Contributor

clason commented May 14, 2021

It is indeed a bit odd. It seems to be stable (not changing depending on which file of the project I open and trigger workspace/symbol from).

The issue is not quite consistent: some indeed don't have a label attached to them:

\@writefile{toc}{\defcounter {refsection}{0}\relax }\@writefile{toc}{\contentsline {section}{\numberline {22.1}Semi-differentiability}{296}{section.1255}\protected@file@percent }

(which looks like the old version parsed?); others do:

\newlabel{eq:graphical:indicator:subdiff}{{20.11}{283}{\MakeLowercase {Derivatives and coderivatives of subdifferentials}}{equation.1208}{}}

Hmm, I can reproduce them in the old arXiv version as well. Maybe it's a problem with the platform (macOS Intel; texlive2021 fully updated)? Here's the full diff (master vs PR) for the output of workspace/symbol (with an empty query):

5d4
< style.tex|31 col 3| [thm] Example
865a865
> monotone.tex|63 col 1| [float] Figure 6.1: Illustration of \cref{ex:inner-outer-difference} with $\limsup_{n \to \infty} X_n=[0, 1]$ while $\liminf_{n \to \infty} X_n=\emptyset$.
988c988
< monotone.tex|741 col 9| [item] (iii)
---
> monotone.tex|741 col 9| [item] Item
1630c1630
< gap.tex|681 col 5| [equ] Equation (11.31)
---
> gap.tex|681 col 5| [equ] Equation
1634c1634
< gap.tex|712 col 1| [sec] 11.32 Duality gap realignment
---
> gap.tex|712 col 1| [sec] Duality gap realignment
2136c2136
< nlpdps.tex|224 col 1| [sec] 15.17 Convergence proof
---
> nlpdps.tex|224 col 1| [sec] Convergence proof
2680c2680
< setderiv.tex|898 col 9| [equ] Equation (20.12)
---
> setderiv.tex|898 col 9| [equ] Equation
2690c2690
< setderiv.tex|949 col 1| [float] Figure 20.3: Illustration of the different graphical derivatives and coderivatives of $\subdiff f$ for $f=\abs{\freevar}$. The dashed line is $\graph \subdiff f$. The dots indicate the base points $(x, y)$ where $D[\subdiff f](x|y)$ is calculated, and the thick arrows and filled-in areas the directions of $(\Delta x, \Delta y)$ (resp.~$(\Delta x, -\Delta y)$ for the coderivatives) relative to the base point.  Observe that there is no graphical regularity at $(x, y)\in\{(0, -1), (0, 1)\}$. Everywhere else, $\subdiff f$ is graphically regular.  Observe that cones in the last figures of each row are polar to the cones in the first and the second figures on the same row.
---
> setderiv.tex|949 col 1| [float] Figure 20.3: % Illustration of the different graphical derivatives and coderivatives of $\subdiff f$ for $f=\abs{\freevar}$. The dashed line is $\graph \subdiff f$. The dots indicate the base points $(x, y)$ where $D[\subdiff f](x|y)$ is calculated, and the thick arrows and filled-in areas the directions of $(\Delta x, \Delta y)$ (resp.~$(\Delta x, -\Delta y)$ for the coderivatives) relative to the base point.  Observe that there is no graphical regularity at $(x, y)\in\{(0, -1), (0, 1)\}$. Everywhere else, $\subdiff f$ is graphically regular.  Observe that cones in the last figures of each row are polar to the cones in the first and the second figures on the same row.
2779c2779
< graphical.tex|11 col 1| [sec] 22.1 Semi-differentiability
---
> graphical.tex|11 col 1| [sec] Semi-differentiability
2878c2878
< cofrechet.tex|64 col 5| [equ] Equation
---
> cofrechet.tex|64 col 5| [equ] Equation (23.3)
2943c2943
< gclarke.tex|6 col 1| [sec] 24.1 Strict differentiability
---
> gclarke.tex|6 col 1| [sec] Strict differentiability

@pfoerster
Copy link
Member Author

Hmm, I can reproduce them in the old arXiv version as well.

Hmm, that's very odd. I cannot reproduce any of these examples (except for gap.tex where I cannot find the section label). Is hovering over the labels affected as well by the issue? Another question: where are the .aux files located in your setup? Are they inside the root folder or not? For example, in gclarke.tex, line 6, I can hover over \label{chap:cofrechet} and see the label number because of

\newlabel{chap:gclarke}{{24}{308}{Calculus for the Clarke graphical derivative}{chapter.1315}{}}

in the aux file. Does your aux file have the same entry or is there some difference?

@clason
Copy link
Contributor

clason commented May 14, 2021

Are you sure you haven't mixed up the labels? I don't have that label in my gclarke.tex (it's on line 2 of cofrechet.tex, and the aux file has \newlabel{chap:cofrechet}{{23}{306}{Calculus for the Fr{\'e}chet coderivative}{chapter.1286}{}} -- there is no label for section 22.1). But, yes, I can hover over it. The aux files are in the project root (I don't mess with build directories; that only end in tears...)

(And the fact that it shows the number without label confused me, too.)

But something is screwy with the equation (20.12), anyway -- it should be (20.11), and the entry points towards the cases environment, not the equation environment. So it might be that this PR is actually more correct! (The section number 15.17 seems pretty suspect, now that I think of it...)

One thing that may explain this is that line 712 of gap.tex starts a subequations environment:

\begin{subequations}
\label{eq:gap:accel:setup}                                                                     
    \begin{equation}                                                                                                  
      \Happrox_{k+1}(u)
      \defeq \Step_{k+1}(\subdiff \tilde G(\nextu)+\grad \tilde F(\thisu)+\Xi)
    \end{equation}
...
\end{subequations}

This is incorrectly picked up as a section -- maybe the parser is confused by this environment and erroneously attaches that label to the last token it does recognize, which is the \section? (Same for the nlpdps, graphical and gclarke labels, actually!)

And the cofrechet label has the possible issue that it is eq:graphical:frechetnormal-outer-composition-linear:1,0 -- possibly the comma is confusing the parser?

I've added two logs for textDocument/symbol for setderiv.tex in case that helps. I also attach all my aux files.

Archive.zip
aux_files.zip

@pfoerster
Copy link
Member Author

Are you sure you haven't mixed up the labels? I don't have that label in my gclarke.tex (it's on line 2 of cofrechet.tex, and the aux file has \newlabel{chap:cofrechet}{{23}{306}{Calculus for the Fr{'e}chet coderivative}{chapter.1286}{}} -- there is no label for section 22.1).

Hmm, it looks like we have different versions of the same file.

One thing that may explain this is that line 712 of gap.tex starts a subequations environment:

That definitely explains it. Subequations was not recognized yet and the label rendering function attaches the label to the section instead.

And the cofrechet label has the possible issue that it is eq:graphical:frechetnormal-outer-composition-linear:1,0 -- possibly the comma is confusing the parser?

That's an issue. The comma is confusing the parser because it cannot determine at the parsing stage whether these are two labels or just one label with a comma in its name. I have to think of a way to properly deal with this.

@clason
Copy link
Contributor

clason commented May 15, 2021

Hmm, it looks like we have different versions of the same file.

Possible; I'm using the latest version (v3, https://arxiv.org/abs/2001.00216v3).

That definitely explains it. Subequations was not recognized yet and the label rendering function attaches the label to the section instead.

👍🏻

That's an issue. The comma is confusing the parser because it cannot determine at the parsing stage whether these are two labels or just one label with a comma in its name. I have to think of a way to properly deal with this.

Hmm. At least \cref allows multiple labels, so this is definitely an issue. In any case, it's easy to fix (the parser is not that confused that rename doesn't work ;)) by avoiding such an "illegal" label, so I'd consider this a wontfix.

So I'd say the only remaining "regresssion" is cosmetic (master strips the % plus linebreak at the beginning of the caption of Figure 20.3, while this PR does not), but that should not hold up merging this PR!

docs/options.md Outdated Show resolved Hide resolved
.unwrap()
.formatter_line_length
.map(|value| {
if value < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be <= 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the fix should be coming soon along with the newlines fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

(If the newline stripping was intentional, that's fine of course.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... Something is still weird. It uses the default value when I format first; if I undo and format again, it uses the correct value...

(This might be a client issue, though.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that seems to have done the trick, thank you! 👍🏻

(sorry for bugging you with all these small things... I'll shut up now ;))

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem, we fixed a lot of (smaller) bugs, now. I think, we can merge this now, can't we?

By the way, what do you think about this feature?
I was thinking about updating the indexing script in order to allow completion of key value options (mainly package imports and other commands that allow (x)keyval arguments. The parser is now capable of handling these, so I guess it makes sense to make some use of it. One could even go further and provide warnings using that information if you try to use some unknown key, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, we fixed a lot of (smaller) bugs, now. I think, we can merge this now, can't we?

Yes, I think so, too!

By the way, what do you think about this feature?
I was thinking about updating the indexing script in order to allow completion of key value options (mainly package imports and other commands that allow (x)keyval arguments. The parser is now capable of handling these, so I guess it makes sense to make some use of it. One could even go further and provide warnings using that information if you try to use some unknown key, for example.

Oh, that's an interesting idea! Yes, I can see how that could be useful. You'd add [ and , as a trigger character for completion, then (assuming the latter isn't already)? Is that something you'd be able to support generally, or do you have to add these explicitly as a json?

(If you really want to push the linting, you could also add type information to keys so you could show an error when passing a bool for an integer value...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that something you'd be able to support generally, or do you have to add these explicitly as a json?

This can be supported generically using the latex-completion-data script as a basis. We still have an (incomplete) Rust port around, which makes the script incremental and faster to execute. We can extend this script to detect commands like \define@key or \define@boolkey, which allows us to provide completion for them without hard-coding every single package (which is basically impossible).

@pfoerster pfoerster merged commit cc77103 into master May 15, 2021
@pfoerster pfoerster deleted the breaking-changes branch June 22, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment