emacs-languagetool / flymake-languagetool

Flymake support for LanguageTool
GNU General Public License v3.0
33 stars 11 forks source link

Fixing errors in narrowed buffers #14

Closed krisbalintona closed 1 year ago

krisbalintona commented 1 year ago

I am on Emacs 30.0.50 (built from master) and the current behavior when calling flymake-start in a narrowed buffer (while flymake-languagetool--checker is a backend) results in an error. The following is a sample backtrace:

Debugger entered--Lisp error: (args-out-of-range 323 323)
  get-text-property(323 face #<buffer todo.org>)
  (let ((x (get-text-property pos 'face src-buf))) (cl-intersection faces-to-ignore (ensure-list x)))
  flymake-languagetool--ignore-at-pos-p(323 #<buffer todo.org> (org-code org-block))
  flymake-languagetool--check-all((((message . "It seems like there are too many consecutive space...") (shortMessage . "") (replacements ((value . " "))) (offset . 322) (length . 2) (context (text . "...2023-01-19 Thu 20:23] =>  0:40 :END: The  possi...") (offset . 43) (length . 2)) (sentence . "* DONE [#A] Confirm which classes I want to take  ...") (type (typeName . "Other")) (rule (id . "CONSECUTIVE_SPACES") (subId . "1") (sourceFile . "grammar.xml") (description . "Two consecutive spaces") (issueType . "typographical") (category (id . "TYPOGRAPHY") (name . "Typography"))) (ignoreForIncompleteSentence . t) (contextForSureMatch . -1)) ((message . "The word ‘sure”s’ is not standard English. Did you...") (shortMessage . "Possible typo") (replacements ((value . "sure’s")) ((value . "sure's"))) (offset . 716) (length . 6) (context (text . "...y Continental Rationalism      The “for sure”s ...") (offset . 43) (length . 6)) (sentence . "The “for sure”s are:\n1.") (type (typeName . "Other")) (rule (id . "NON_STANDARD_WORD") (subId . "1") (sourceFile . "grammar.xml") (description . "Non-standard word") (issueType . "misspelling") (category (id . "TYPOS") (name . "Possible Typo"))) (ignoreForIncompleteSentence . t) (contextForSureMatch . 10)) ((message . "You used an adverb (‘intellectually’) instead of a...") (shortMessage . "Incorrect adverb") (replacements ((value . "intellectual interest"))) (offset . 1392) (length . 23) (context (text . "...on and film watching sessions...)   The intelle...") (offset . 43) (length . 23)) (sentence . "The intellectually interest but heavy ones are:\n1.") (type (typeName . "Other")) (rule (id . "A_RB_NN") (subId . "1") (sourceFile . "grammar.xml") (description . "Adverb instead of an adjective") (issueType . "grammar") (urls ((value . "http://www.ucl.ac.uk/internet-grammar/adverbs/xadv..."))) (category (id . "GRAMMAR") (name . "Grammar"))) (ignoreForIncompleteSentence . t) (contextForSureMatch . 6)) ((message . "Use a comma before ‘so’ if it connects two indepen...") (shortMessage . "") (replacements ((value . ", so"))) (offset . 1667) (length . 3) (context (text . "...tions About Rational Belief*, the former so I c...") (offset . 43) (length . 3)) (sentence . "So the plan is that I for sure take *UNIV1520 The ...") (type (typeName . "Other")) (rule (id . "COMMA_COMPOUND_SENTENCE") (subId . "1") (sourceFile . "grammar.xml") (description . "comma between independent clauses") (issueType . "typographical") (urls ((value . "https://www.grammar-monster.com/lessons/commas_bef..."))) (category (id . "PUNCTUATION") (name . "Punctuation"))) (ignoreForIncompleteSentence . t) (contextForSureMatch . -1)) ((message . "Three successive sentences begin with the same wor...") (shortMessage . "Three successive sentences begin with the same wor...") (replacements) (offset . 2307) (length . 2) (context (text . "...ntury Continental Rationalism* over it. If I do...") (offset . 43) (length . 2)) (sentence . "If I don't end up liking either *ANTH0100 Introduc...") (type (typeName . "Other")) (rule (id . "ENGLISH_WORD_REPEAT_BEGINNING_RULE") (description . "Successive sentences beginning with the same word") (issueType . "style") (category (id . "STYLE") (name . "Style"))) (ignoreForIncompleteSentence . t) (contextForSureMatch . -1)) ((message . "Three successive sentences begin with the same wor...") (shortMessage . "Three successive sentences begin with the same wor...") (replacements) (offset . 2520) (length . 2) (context (text . "... Theories of Modern Culture and Media*. If I do...") (offset . 43) (length . 2)) (sentence . "If I don't like both, then I'll think of sticking ...") (type (typeName . "Other")) (rule (id . "ENGLISH_WORD_REPEAT_BEGINNING_RULE") (description . "Successive sentences beginning with the same word") (issueType . "style") (category (id . "STYLE") (name . "Style"))) (ignoreForIncompleteSentence . t) (contextForSureMatch . -1))) #<buffer todo.org>)
  flymake-languagetool--output-to-errors("\n{\"software\":{\"name\":\"LanguageTool\",\"version\":\"6.0..." #<buffer todo.org>)
  flymake-languagetool--handle-finished(nil #<buffer todo.org> #f(compiled-function (&rest args) #<bytecode 0xead5fdec5a8084a>))
  url-http-activate-callback()
  url-http-content-length-after-change-function(105 6042 5937)
  url-http-wait-for-headers-change-function(1 6047 6046)
  url-http-generic-filter(#<process localhost> "HTTP/1.1 200 OK\15\nDate: Fri, 20 Jan 2023 09:17:05 G...")

The error seems to come from flymake-languagetool--ignore-at-pos-p trying to get the face of a character whose position lies outside of the narrowed buffer.

Wrapping the contents of that function in a when clause that checks that pos is within the narrowed buffer (e.g. (and (<= (point-min) pos) (<= pos (point-max)) seems to at least remove the errors on my end:

(defun flymake-languagetool--ignore-at-pos-p (pos src-buf
                                                       faces-to-ignore)
    "Return non-nil if faces at POS in SRC-BUF intersect FACES-TO-IGNORE."
    (when (and (<= (point-min) pos) (<= pos (point-max)))
      (let ((x (get-text-property pos 'face src-buf)))
        (cl-intersection faces-to-ignore (ensure-list x)))))
jcs090218 commented 1 year ago

Yeah, we haven't handle the narrowed buffer for this package. @tpeacock19 Do you have time for this? 😕

Some references:

tpeacock19 commented 1 year ago

Yes will do so this evening.

tpeacock19 commented 1 year ago

I'm not able to replicate this issue. It seems to work already with narrowed buffers for me, probably due to this:

https://github.com/emacs-languagetool/flymake-languagetool/blob/d995f90aac2d10baa617f4de5c6e40bc37e63f76/flymake-languagetool.el#L317-L319

However, I have committed a change (https://github.com/emacs-languagetool/flymake-languagetool/commit/718b71ad1beabd7318df7b325f7316a24a821ef9) that I think should fix your issue. @krisbalintona, Can you please check if this fixes your issue or provide a sample file you get this error with, so I can continue troubleshooting?

krisbalintona commented 1 year ago

I looked at the code and also expected narrowing to work after I noticed those lines. Nevertheless, it did error for me.

I'd try replicating the issue with emacs -Q, but I just upgraded the package after the latest commit you've pushed, and so the issue has been resolved. I'll report back if it comes up again.

Thank you for your work!