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

spell/nospell doesn't work properly in optional parameter for theorem-like environment #2086

Closed
tornaria opened this issue Jun 30, 2021 · 14 comments

Comments

@tornaria
Copy link
Contributor

Inside optional parameters for theorem-like environment and also other macros (notable exception I could find: \title, see below), text is usually nospell. Moreover, the arguments of a \ref{....} or \cite{...} inside this optional parameter are spell.

Desired behaviour:

  • text in optional parameter for theorem-like environment is spell-checked
  • but text inside \ref{...} or \cite{...} is never spell-checked

To try, just load the file below and :set spell.

\documentclass{article}
\begin{document}

This is good \ref{abc}

\begin{theorem}[This is ignored abc and this is bad \ref{abc}]
    My theorem
\end{theorem}


More testing:

\centerline{This is good \ref{abc}}

\title[This is good \ref{abc}]{This is good \ref{abc}}
\author[This is bad \ref{abc}]{This is good \ref{abc}}
\usepackage[This is bad \ref{abc}]{This is bad \ref{abc}}

\mycommand[This is ignored: abc and this is bad: \ref{abc}]{This is good \ref{abc}}

\end{document}
@lervag
Copy link
Owner

lervag commented Jun 30, 2021

This needs more refinement. Different commands work differently. The \usepackage command should not have a "textual" option group, so I won't change that, and you can't use the \ref command inside the \usepackage arg. The \title and \author commands do not by default have any option groups. Custom commands will never be perfectly handled. And theorem is not a valid command unless you use a package.

So, to move forward, we need to be more specific. I.e., for theorem, please provide the relevant package where the environment is defined. Similar with the option groups for the \title and \author commands - which package or class do you use that redefine these commands to accept an optional argument? I would also prefer to see the documentation references for these things.

Text inside \ref and \cite is not spell-checked in general, but within nested pairs of {...} (e.g. command arguments) we need to handle things individualle per command. Thus, in some cases, the nesting will break the syntax highlighting. This will simply never be perfect, but I try to make it as good as possible.

@tornaria
Copy link
Contributor Author

tornaria commented Jun 30, 2021

Thanks so much for your quick answer. I will try be more specific on theorem-like environments, which is the more important issue IMHO.

I redid the example, I hope it's better explained now. This example compiles without any warning and no external packages.

\documentclass{article}

% Theorem-like environment is introduced by \newtheorem
% - first mandatory argument is the code-name for the environment
%   should NOT be spell-checked (but it currently is)
% - optional argument after first mandatory argument is the code-name
%   of the counter to use, should NOT be spell-checked (but it
%   currently is)
% - second mandatory argument is the name to print, so that one SHOULD
%   BE spell checked (as it currently is OK)
% - optional argument after second mandatory argument is the code-name
%   of a counter (in this case it means number theorems by sections)
%   so it should NOT be spell-checked (it currently is)
\newtheorem{thm}{Theorem}[section]
\newtheorem{prp}[thm]{Proposition}

\begin{document}

% Theorem-like environment defined above, code-name "thm"
% The optional argument should be spell-checked, but not if it is
% inside \ref{...}, \cite{...}, etc.
%
% Currently: the optional argument is not spell-checked, and it also
% disables the nospell attribute for \ref{...}, \cite{...}, etc.
\begin{thm}[description of theorem abc \cite{abc}]
    My theorem goes here.
\end{thm}

% Just to ilustrate another theorem-like environment
\begin{prp}[pleaze spelll check]
    My proposition goes here.
\end{prp}

\begin{thebibliography}{1}
% BTW, "abc" here should not be spell-checked (it is)
\bibitem{abc} a citation.
\end{thebibliography}

\end{document}

EDIT: added 'optional argument after second mandatory argument' for \newtheorem command.

Documentation for \newtheorem is here: https://en.m.wikibooks.org/wiki/LaTeX/Theorems.

@tornaria
Copy link
Contributor Author

tornaria commented Jul 2, 2021

Here's my attempt at an implementation for the \newtheorem command:

syn match texCmdNewthm "\\newtheorem" nextgroup=texNewthmArg1 contains=texCmd
call vimtex#syntax#core#new_arg('texNewthmArg1', {'next': 'texNewthmOpt1,texNewthmArg2', 'contains': 'TOP,@Spell'})
call vimtex#syntax#core#new_opt('texNewthmOpt1', {'next': 'texNewthmArg2'})
call vimtex#syntax#core#new_arg('texNewthmArg2', {'next': 'texNewthmOpt2'})
call vimtex#syntax#core#new_opt('texNewthmOpt2')
highlight def link texNewthmArg1 texArg
highlight def link texNewthmOpt1 texOpt
highlight def link texNewthmOpt2 texOpt

With this in place, only the second mandatory argument for \newtheorem is spell-checked.

As for the theorem-like environments themselves, which is the most important issue, I realized that all optional arguments for environments are treated as nospell. I would argue this is incorrect, because false negatives are worse than false positives. My preference would be to make optional arguments default to spell, and add exceptions for commands and environments that should have nospell optional arguments. Would this be ok?

As an (imperfect) workaround I'm using this

" environments whose arguments should be spell-checked
for s:env in g:vimtex_syntax_spell_environments
    exec 'syn match texCmdEnv "\\begin{' . s:env . '}" contains=texCmdEnv'
endfor

together with the following config

let g:vimtex_syntax_spell_environments = [
    \ "proof", 
    \ "thm",
    \ "prp",
    \ ]

The problem of this is that one has to add to g:vimtex_syntax_spell_environments any theorem-like env that is declared, and if I forget one this will not be spell-checked (this is a problem when using this to spell-check tex files provided by other authors, which will have many variations on the names used for theorem-like environments). Better to have code to search for \newtheorem at init to figure out which theorem-like envs have been declared, but I'm not sure that's ok.

It would really feel much safer to default to spell with explicit exceptions. Also, a global array g:vimtex_syntax_nospell_environments could be provided so that users can add its own exceptions just as g:vimtex_syntax_nospell_commands.

lervag added a commit that referenced this issue Jul 24, 2021
lervag added a commit that referenced this issue Jul 24, 2021
lervag added a commit that referenced this issue Jul 24, 2021
@lervag
Copy link
Owner

lervag commented Jul 24, 2021

I think I've resolved this issue now, please test with latest version.

@lervag lervag closed this as completed Jul 24, 2021
@lervag
Copy link
Owner

lervag commented Jul 24, 2021

Please note: The optional group for the theorem environments depend on the amsthm package, and so are only available when that package is detected.

@tornaria
Copy link
Contributor Author

tornaria commented Jul 26, 2021

Please note: The optional group for the theorem environments depend on the amsthm package, and so are only available when that package is detected.

Thanks a lot!

Note that the optional group for theorem environments doesn't depend on amsthm, even though the feature is not mentioned in "LaTeX 2e Unofficial reference guide", section 12.9. It is mentioned in "The Not So Short Introduction to LATEX2ε", section 3.9 (AKA "lshort").

In fact, in test/test-syntax/test-core.tex line 114 there's a theorem environment with optional group and it is currently not spell-checked

@tornaria
Copy link
Contributor Author

The one other thing related to theorems is the proof environment, which works like this:

\begin{proof}[Proooof of theorem]
    Here goes the proof.
\end{proof}

As mentioned in "lshort", sect 3.9, this is available when amsthm is loaded.

Should I open a different issue for this one? The following seems to work:

syntax match texProofEnvBegin "\\begin{proof}"
    \ skipwhite skipnl
    \ nextgroup=texProofEnvOpt
    \ contains=texCmdEnv
call vimtex#syntax#core#new_opt('texProofEnvOpt', {
    \ 'contains': 'TOP,@NoSpell'
    \})
highlight def link texProofEnvOpt texEnvOpt

@lervag
Copy link
Owner

lervag commented Jul 26, 2021

Note that the optional group for theorem environments doesn't depend on amsthm, even though the feature is not mentioned in "LaTeX 2e Unofficial reference guide", section 12.9. It is mentioned in "The Not So Short Introduction to LATEX2ε", section 3.9 (AKA "lshort").

Ah, ok. I'm pushing a change to fix this now. I'm also changing the reference to the lshort manual, as it seems more complete.

Note, in my first version I included a minor cache (which works in multi-file projects). I was not able to do that now. By moving it to core, I also moved code that parses the preamble into core. This may give a noticable impact on load times. I've checked and did not measure anything, but its hard to be sure. If I do find this negatively impacts load times, then I might consider to move this back to amsthm to improve load times for most people.

In fact, in test/test-syntax/test-core.tex line 114 there's a theorem environment with optional group and it is currently not spell-checked

Heh, nice catch.

The one other thing related to theorems is the proof environment, which works like this:

Thanks, this should work now.

lervag added a commit that referenced this issue Jul 26, 2021
lervag added a commit that referenced this issue Jul 26, 2021
@tornaria
Copy link
Contributor Author

Again, thanks a lot for the quick work!

Note, in my first version I included a minor cache (which works in multi-file projects). I was not able to do that now. By moving it to core, I also moved code that parses the preamble into core. This may give a noticable impact on load times. I've checked and did not measure anything, but its hard to be sure. If I do find this negatively impacts load times, then I might consider to move this back to amsthm to improve load times for most people.

If you have to disable this by default, could you offer an option so one can choose to reenable?

@clason
Copy link
Contributor

clason commented Jul 27, 2021

Note, in my first version I included a minor cache (which works in multi-file projects). I was not able to do that now. By moving it to core, I also moved code that parses the preamble into core. This may give a noticable impact on load times. I've checked and did not measure anything, but its hard to be sure. If I do find this negatively impacts load times, then I might consider to move this back to amsthm to improve load times for most people.

Ah, I guess that's the reason why the last commit broke syntax highlighting for me if I use a fuzzy finder to open files... (see comment on the last commit, which is the breaking one -- if I revert only that, things work again).

lervag added a commit that referenced this issue Jul 27, 2021
@lervag
Copy link
Owner

lervag commented Jul 27, 2021

Not unexpected to see that this thing broke. @tornaria I've reverted this for now by simply disabling it. I think it is necessary to find a more reliable way to do this, so I will reopen this issue and figure out a better way. In the meantime, I hope you don't mind that I've removed the functionality.

@clason
Copy link
Contributor

clason commented Jul 27, 2021

Yep, works again, thanks :)

@lervag
Copy link
Owner

lervag commented Jul 27, 2021

For my own reference: See #2120 for a simple way to test for errors with the functionality. My current idea is that we need to allow some syntax rules to be applied after the filetype plugin has initialized. So, something like this:

  • Main syntax rules are sourced.
  • VimTeX state is initialized - we can optimize by collecting theorem envs during initialization here.
  • Either from PostInit hook or with after/syntax/tex.vim: Source additional rules that depend on the state.

@lervag lervag reopened this Jul 27, 2021
lervag added a commit that referenced this issue Aug 26, 2021
@lervag
Copy link
Owner

lervag commented Aug 26, 2021

Ok, now the custom theorems should work again. I believe this is implemented more robust now and the previously mentioned issues should be resolved.

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

No branches or pull requests

3 participants