From c232c5f9dba926d8a80f0ebfe620fdfa7bfcbcbf Mon Sep 17 00:00:00 2001 From: Jackson Ray Hamilton
Date: Sat, 9 Feb 2019 20:06:29 -0800 Subject: [PATCH] Refactor JSX indentation code to improve enclosing JSX discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a number of bugs reported for JSX indentation (caused by poor JSX detection): - https://github.com/mooz/js2-mode/issues/140#issuecomment-166250016 - https://github.com/mooz/js2-mode/issues/490 - Bug#24896 / https://github.com/mooz/js2-mode/issues/389 (with respect to comments) - Bug#26001 / https://github.com/mooz/js2-mode/issues/389#issuecomment-271869380 - https://github.com/mooz/js2-mode/issues/411 / Bug#27000 / https://github.com/mooz/js2-mode/issues/451 Potentially manifest some new bugs (due to false positives with ‘<’ and ‘>’ and SGML detection). Slow down indentation a fair bit. * list/progmodes/js.el (js-jsx-syntax, js--jsx-start-tag-re) (js--looking-at-jsx-start-tag-p, js--looking-back-at-jsx-end-tag-p): New variables and functions. (js--jsx-find-before-tag, js--jsx-after-tag-re): Deleted. (js--looking-at-operator-p): Don’t mistake a JSXOpeningElement for the ‘<’ operator. (js--continued-expression-p): Don’t mistake a JSXClosingElement as a fragment of a continued expression including the ‘>’ operator. (js--as-sgml): Simplify. Probably needn’t bind forward-sexp-function to nil (sgml-mode already does) and probably shouldn’t bind parse-sexp-lookup-properties to nil either (see Bug#24896). (js--outermost-enclosing-jsx-tag-pos): Find enclosing JSX more accurately than js--jsx-find-before-tag. Use sgml-mode’s parsing logic, rather than unreliable heuristics like paren-wrapping. This implementation is much slower; the previous implementation was fast, but at the expense of accuracy. To make up for all the grief we’ve caused users, we will prefer accuracy over speed from now on. That said, this can still probably be optimized a lot. (js--jsx-indented-element-p): Rename to js--jsx-indentation, since it doesn’t just return a boolean. (js--jsx-indentation): Refactor js--jsx-indented-element-p to simplify the implementation as the improved accuracy of other code allows (and to repent for some awful stylistic choices I made earlier). (js--expression-in-sgml-indent-line): Rename to js--indent-line-in-jsx-expression, since it’s a private function and we can give it a name that reads more like English. (js--indent-line-in-jsx-expression): Restructure point adjustment logic more like js-indent-line. (js--indent-n+1th-jsx-line): New function to complement js--indent-line-in-jsx-expression. (js-jsx-indent-line): Refactor. Don’t bind js--continued-expression-p to ignore any more; instead, rely on the improved accuracy of js--continued-expression-p. (js-jsx-mode): Set js-jsx-syntax to t. For now, this will be the flag we use to determine whether ‘JSX is enabled.’ (Maybe later, we will refactor the code to use this variable instead of requiring js-jsx-mode to be enabled, thus rendering the mode obsolete.) --- lisp/progmodes/js.el | 337 +++++++++++++++++++++------------------------------ 1 file changed, 141 insertions(+), 196 deletions(-) diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el index b0bb8213dc..e16ed98023 100644 --- a/lisp/progmodes/js.el +++ b/lisp/progmodes/js.el @@ -572,6 +572,15 @@ js-chain-indent :safe 'booleanp :group 'js) +(defcustom js-jsx-syntax nil + "When non-nil, parse JavaScript with consideration for JSX syntax. +This fixes indentation of JSX code in some cases. It is set to +be buffer-local when in `js-jsx-mode'." + :version "27.1" + :type 'boolean + :safe 'booleanp + :group 'js) + ;;; KeyMap (defvar js-mode-map @@ -1774,6 +1783,14 @@ js--indent-operator-re (js--regexp-opt-symbol '("in" "instanceof"))) "Regexp matching operators that affect indentation of continued expressions.") +(defconst js--jsx-start-tag-re + (concat "<" sgml-name-re) + "Regexp matching code that looks like a JSXOpeningElement.") + +(defun js--looking-at-jsx-start-tag-p () + "Non-nil if a JSXOpeningElement immediately follows point." + (looking-at js--jsx-start-tag-re)) + (defun js--looking-at-operator-p () "Return non-nil if point is on a JavaScript operator, other than a comma." (save-match-data @@ -1796,7 +1813,9 @@ js--looking-at-operator-p (js--backward-syntactic-ws) ;; We might misindent some expressions that would ;; return NaN anyway. Shouldn't be a problem. - (memq (char-before) '(?, ?} ?{)))))))) + (memq (char-before) '(?, ?} ?{))))) + ;; “<” isn’t necessarily an operator in JSX. + (not (and js-jsx-syntax (js--looking-at-jsx-start-tag-p)))))) (defun js--find-newline-backward () "Move backward to the nearest newline that is not in a block comment." @@ -1816,6 +1835,14 @@ js--find-newline-backward (setq result nil))) result)) +(defconst js--jsx-end-tag-re + (concat "" sgml-name-re ">\\|/>") + "Regexp matching a JSXClosingElement.") + +(defun js--looking-back-at-jsx-end-tag-p () + "Non-nil if a JSXClosingElement immediately precedes point." + (looking-back js--jsx-end-tag-re (point-at-bol))) + (defun js--continued-expression-p () "Return non-nil if the current line continues an expression." (save-excursion @@ -1833,12 +1860,19 @@ js--continued-expression-p (and (js--find-newline-backward) (progn (skip-chars-backward " \t") - (or (bobp) (backward-char)) - (and (> (point) (point-min)) - (save-excursion (backward-char) (not (looking-at "[/*]/\\|=>"))) - (js--looking-at-operator-p) - (and (progn (backward-char) - (not (looking-at "+\\+\\|--\\|/[/*]")))))))))) + (and + ;; The “>” at the end of any JSXBoundaryElement isn’t + ;; part of a continued expression. + (not (and js-jsx-syntax (js--looking-back-at-jsx-end-tag-p))) + (progn + (or (bobp) (backward-char)) + (and (> (point) (point-min)) + (save-excursion + (backward-char) + (not (looking-at "[/*]/\\|=>"))) + (js--looking-at-operator-p) + (and (progn (backward-char) + (not (looking-at "+\\+\\|--\\|/[/*]")))))))))))) (defun js--skip-term-backward () "Skip a term before point; return t if a term was skipped." @@ -2153,190 +2187,108 @@ js--proper-indentation ;;; JSX Indentation -(defsubst js--jsx-find-before-tag () - "Find where JSX starts. - -Assume JSX appears in the following instances: -- Inside parentheses, when returned or as the first argument - to a function, and after a newline -- When assigned to variables or object properties, but only - on a single line -- As the N+1th argument to a function - -This is an optimized version of (re-search-backward \"[(,]\n\" -nil t), except set point to the end of the match. This logic -executes up to the number of lines in the file, so it should be -really fast to reduce that impact." - (let (pos) - (while (and (> (point) (point-min)) - (not (progn - (end-of-line 0) - (when (or (eq (char-before) 40) ; ( - (eq (char-before) 44)) ; , - (setq pos (1- (point)))))))) - pos)) - -(defconst js--jsx-end-tag-re - (concat "" sgml-name-re ">\\|/>") - "Find the end of a JSX element.") - -(defconst js--jsx-after-tag-re "[),]" - "Find where JSX ends. -This complements the assumption of where JSX appears from -`js--jsx-before-tag-re', which see.") - -(defun js--jsx-indented-element-p () +(defmacro js--as-sgml (&rest body) + "Execute BODY as if in sgml-mode." + `(with-syntax-table sgml-mode-syntax-table + ,@body)) + +(defun js--outermost-enclosing-jsx-tag-pos () + (let (context tag-pos last-tag-pos parse-status parens paren-pos curly-pos) + (js--as-sgml + ;; Search until we reach the top or encounter the start of a + ;; JSXExpressionContainer (implying nested JSX). + (while (and (setq context (sgml-get-context)) + (progn + (setq tag-pos (sgml-tag-start (car (last context)))) + (or (not curly-pos) + ;; Stop before curly brackets (start of a + ;; JSXExpressionContainer). + (> tag-pos curly-pos)))) + ;; Record this position so it can potentially be returned. + (setq last-tag-pos tag-pos) + ;; Always parse sexps / search for the next context from the + ;; immediately enclosing tag (sgml-get-context may not leave + ;; point there). + (goto-char tag-pos) + (unless parse-status ; Don’t needlessly reparse. + ;; Search upward for an enclosing starting curly bracket. + (setq parse-status (syntax-ppss)) + (setq parens (reverse (nth 9 parse-status))) + (while (and (setq paren-pos (car parens)) + (not (when (= (char-after paren-pos) ?{) + (setq curly-pos paren-pos)))) + (setq parens (cdr parens))) + ;; Always search for the next context from the immediately + ;; enclosing tag (calling syntax-ppss in the above loop + ;; may move point from there). + (goto-char tag-pos)))) + last-tag-pos)) + +(defun js--jsx-indentation () "Determine if/how the current line should be indented as JSX. -Return `first' for the first JSXElement on its own line. -Return `nth' for subsequent lines of the first JSXElement. -Return `expression' for an embedded JS expression. -Return `after' for anything after the last JSXElement. -Return nil for non-JSX lines. - -Currently, JSX indentation supports the following styles: - -- Single-line elements (indented like normal JS): - - var element = ; - -- Multi-line elements (enclosed in parentheses): - - function () { - return ( -