From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#73746: bug#73725: Master: Wrong position for byte compiler warning message. Date: Sun, 20 Oct 2024 12:35:46 -0400 Message-ID: References: <8A05D4D2-C77B-4089-B82C-4DB3E88F1276@gmail.com> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23061"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 73746@debbugs.gnu.org, Mattias =?UTF-8?Q?Engdeg=C3=A5rd?= , 73725@debbugs.gnu.org To: Alan Mackenzie Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Oct 20 18:37:00 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1t2YvK-0005mw-6d for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 20 Oct 2024 18:36:58 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1t2Yv1-0000qA-JV; Sun, 20 Oct 2024 12:36:39 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1t2Yuz-0000pc-NG for bug-gnu-emacs@gnu.org; Sun, 20 Oct 2024 12:36:37 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1t2Yuz-0000gv-Ec for bug-gnu-emacs@gnu.org; Sun, 20 Oct 2024 12:36:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:Date:References:In-Reply-To:From:To:Subject; bh=DrtyCjVtaBW5XV4krtpWo/eiAoVL5+eO9WXs03swxe8=; b=tRBKU1HV/P+ma8bu3t/wwHvNfpm5aQ6fr9jFX6GMF0LdMj7rvKbuiUb29Mi6aYT1Eii9NazGcX5eItj385xwQdvyDt99qHX1XD/tVt4jfbKObNs32Td2wfbXyE+Aq1J1iSbldrACBpYd+5Mo1dVu43h59ockH1FSLjOcHJdg2htwFeLOztQfNeD9K45tdAcwymYn98jzQyJ7Hw+P2u31Ogde7EH1DJy66caUgvN5E6mLWfgXR/gid54vjooJ0sgyQ8vmdUg1TFW6XM9D0Ee+j3TtbMt3kVLkMnnz4MH9oie7xWCv/IkFLjs3lPjLT/Q5S/IARcr5OlpRXJ1NpBPSXw==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1t2YvO-0000o0-N8 for bug-gnu-emacs@gnu.org; Sun, 20 Oct 2024 12:37:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 20 Oct 2024 16:37:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 73746 X-GNU-PR-Package: emacs Original-Received: via spool by 73746-submit@debbugs.gnu.org id=B73746.17294421863031 (code B ref 73746); Sun, 20 Oct 2024 16:37:02 +0000 Original-Received: (at 73746) by debbugs.gnu.org; 20 Oct 2024 16:36:26 +0000 Original-Received: from localhost ([127.0.0.1]:48269 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t2Yun-0000mg-N0 for submit@debbugs.gnu.org; Sun, 20 Oct 2024 12:36:26 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:48280) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t2Yuk-0000lm-QC; Sun, 20 Oct 2024 12:36:23 -0400 Original-Received: from pmg2.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 83A278093F; Sun, 20 Oct 2024 12:35:50 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1729442149; bh=Wr/64k+jRnzAXlarjcUAdwVFuhco4pSk2b/+uDDBP5k=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=aLYH13ew1SJHKBtoO+O96pbL1Ud6t05RGZnr5Ws1RKdsYuNx155Tg8R+66vfhWqqU XOEcS3JLyuLRbqPzmC/RfWq2eCR1ym4kSYeohpI9pa/tClu8YNRRqGqUfBdQy4o2n+ MsJd97dXedLrRJJydxZn4AC2+B1cmPk+P07jH4i9Ml+h4hZ8Y8vh5Bp2fhDe+Cux7F 6dUNSet13Vd2auUXGS85AAmEMNOOWo0yT3amnQJN5BXMryBznBbP5M6tDDCOZm7+QR O2sKQOAJpmZNFEo9qwrR7JVcFFoACOuMNtypNUD3I6qhB+vpwUZiZ2bAQwFIJVgWtM IbCxPZuA2tTaA== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 4218B80088; Sun, 20 Oct 2024 12:35:49 -0400 (EDT) Original-Received: from pastel (69-196-161-60.dsl.teksavvy.com [69.196.161.60]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 0BADC12029A; Sun, 20 Oct 2024 12:35:49 -0400 (EDT) In-Reply-To: (Alan Mackenzie's message of "Sun, 20 Oct 2024 14:13:29 +0000") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:293987 Archived-At: > I haven't heard anything back in over a week, so I assume my patch for > bug#73725 and bug#73746 is OK. I've committed it. Sorry, got pushed too far down my todo while I was busy and forgot about it. Side note: thinking some more about the problem you're trying to fix, I can't help find it is somewhat funny: it's exactly the kind of "position-preservation" work we were hoping to avoid having to do by using SWPs. Anyway, the general approach seems about right. See comments below: > @@ -510,7 +510,9 @@ byte-optimize-form > (while > (progn > ;; First, optimize all sub-forms of this one. > - (setq form (byte-optimize-form-code-walker form for-effect)) > + (setq form > + (macroexp-preserve-posification > + form (byte-optimize-form-code-walker form for-effect))) > > ;; If a form-specific optimizer is available, run it and start over > ;; until a fixpoint has been reached. Is this needed? I'd expect we need `macroexp-preserve-posification` only at those places where an optimization returns a form whose `car` is different from that of FORM. So I expect this to happen in more specific places inside byte-opt.el than here. IOW, this deserves a clear comment explaining why we need it here, probably with some kind of example. > @@ -519,7 +521,8 @@ byte-optimize-form > (let ((opt (byte-opt--fget (car form) 'byte-optimizer))) > (and opt > (let ((old form) > - (new (funcall opt form))) > + (new (macroexp-preserve-posification > + form (funcall opt form)))) > (byte-compile-log " %s\t==>\t%s" old new) > (setq form new) > (not (eq new old)))))))) E.g. this is the kind of place where it makes sense to me. > +(defun sub-macroexp--posify-form (form call-pos depth) Please don't eat up namespace gratuitously. IOW stick to the "macroexp-" prefix. > + "Try to apply the transformation of `macroexp--posify-form' to FORM. > +FORM and CALL-POS are as in that function. DEPTH is a small integer, > +decremented at each recursive call, to prevent infinite recursion. > +Return the changed form, or nil if no change happened." Somewhere in the doc or in a comment we should document the "design", which AFAICT is to try and find "one" place where we can put the `call-pos` info: the docstring suggests we'll apply it to all the symbols within `depth` (which would be both costly and undesirable), whereas we actually stop (more or less) as soon as we find a good spot. > + (let (new-form) > + (cond > + ((zerop depth) nil) > + ((and (consp form) > + (symbolp (car form)) > + (car form)) > + (setcar form (position-symbol (car form) call-pos)) > + form) AFAICT this can and occasionally will throw away valuable position information: if `(car form)` is an SWP we don't know for sure here that `call-pos` is always a better position info than the one already carried by `(car form)`. We could consider combining position information (but this would make the whole system more complex: when printing warnings/errors we'd have to find a way to make use of the various recorded positions, ...). But a simpler choice is to check (not (symbol-with-pos-p (car form))). > + ((symbolp form) > + (if form ; Don't position nil! > + (position-symbol form call-pos))) Same here. > +(defmacro macroexp-preserve-posification (pos-form &rest body) > + "Evaluate BODY..., posifying the result with POS-FORM's position, if any." This lacks a `declare` with `debug` and `indent` specs. > + `(let ((call-pos (cond > + ((consp ,pos-form) > + (and (symbol-with-pos-p (car ,pos-form)) > + (symbol-with-pos-pos (car ,pos-form)))) > + ((symbol-with-pos-p ,pos-form) > + (symbol-with-pos-pos ,pos-form)))) > + (new-value (progn ,@body))) > + (if call-pos > + (macroexp--posify-form new-value call-pos) > + new-value))) > + > (defun macroexp-macroexpand (form env) > "Like `macroexpand' but checking obsolescence." > (let* ((macroexpand-all-environment env) > new-form) > + (macroexp-preserve-posification > + form > (while (not (eq form (setq new-form (macroexpand-1 form env)))) > + (setq macroexpanded t) > (let ((fun (car-safe form))) > (setq form > (if (and fun (symbolp fun) This `(setq macroexpanded t)` looks like some leftover code, at least I couldn't find this var declared or used elsewhere. But yes, I suspect it would make a fair bit of sense to perform the "preserve-posification" only when `macroexpand-1` did return a new form. Did you try to do it (as suggested by this leftover code) and found it was not worth the trouble or that it didn't work? If so, please add a comment recording it. > @@ -253,7 +316,7 @@ > (if (symbolp (symbol-function fun)) "alias" "macro")) > new-form (list 'obsolete fun) nil fun) > new-form)))) > - form)) > + new-form))) Why? > -(defun macroexpand-all (form &optional environment) > +(defun macroexpand-all (form &optional environment keep-pos) Any reason why we need this new argument? Can't we just always try to preserve the positions? Stefan