| Message ID | 875yvc4254.fsf_-_@gnu.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#50286,RFC] Let 'package-location' returns location of surrounding 'let'. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
Hi Ludo, Ludovic Courtès <ludo@gnu.org> writes: > Example: > > scheme@(guile-user)> ,use(gnu packages base) > scheme@(guile-user)> ,use(gnu packages accessibility) > scheme@(guile-user)> ,use(guix) > scheme@(guile-user)> (package-location footswitch) > $1 = #<<location> file: "gnu/packages/accessibility.scm" line: 257 column: 4> > scheme@(guile-user)> (package-definition-location footswitch) > $2 = #<<location> file: "gnu/packages/accessibility.scm" line: 254 column: 0> > scheme@(guile-user)> (package-location hello) > $3 = #<<location> file: "gnu/packages/base.scm" line: 79 column: 2> > scheme@(guile-user)> (package-definition-location hello) > $4 = #<<location> file: "gnu/packages/base.scm" line: 78 column: 0> > > Thoughts? This is very clever! Thanks for the work on this. I'm not very good with macros, but it *looks* like it should work quite well for our use-case of adjusting a surrounding 'let' expression. And it's less invasive than rewriting 'let'. However... it doesn't work for unexported packages. It looks there are about 200 such packages: --8<---------------cut here---------------start------------->8--- ~/guix$ rg -U '\(define [^\(]+\n.*?\(package' gnu/packages --count --no-filename | awk '{a+=$1} END {print a}' 233 --8<---------------cut here---------------end--------------->8--- And, to play the pessimist: What do we get out of this that couldn't be done by "go to package location; read backwards one sexp until we reach a defining form" (like Emacs' 'beginning-of-defun')? -- Sarah
Ludovic Courtès schreef op di 07-09-2021 om 21:27 [+0200]: > Hi Maxime & Sarah, > > Ludovic Courtès <ludo@gnu.org> skribis: > > > Hmm, thinking out loud, what about this: use the same trick as you did, > > but replace ‘define-public’ instead of ‘let’ & co., so as to be less > > intrusive. > > > > (define-syntax-parameter current-definition-location > > (identifier-syntax #f)) > > > > (define-syntax define-public* > > (syntax-rules () > > ((_ prototype body) > > (define-public prototype > > (syntax-parameterize ((current-definition-location > > (identifier-syntax (current-source-location)))) > > body))))) > > > > Since there’s code that assumes ‘package-location’ returns the location > > of the (package …) sexp, we could add a ‘definition-location’ field in > > <package>, defaulting to ‘current-definition-location’, or tweak > > ‘location’ to include both. > > Below is an attempt at doing this. As discussed on IRC, the first patch > switches the ‘location’ field to a more compact format that may reduce > load time by a tiny bit, though it’s hard to measure. > The second patch > introduces an extra field for the definition location; that means that > <package> records now occupy an extra word, which is not great, but > unfortunately OTOH location is slightly smaller. Why not always let the location of a package be the location of the surrounding define-public* form, instead of having two separate locations? Letting the location of a package be the location of the define-public* form (or 'let' form) seems more useful to people using "guix edit minetest-etheral" for example, and the package-field-location code can easily be adjusted to support 'define-public*' (or let) forms. If two separate package-definition-location and package-location are introduced, what should "guix show minetest-ethereal" show? The location of the 'package' form, the location of the 'let' form or the location of the 'define-public' form? Having two separate define-public* and define-public macros might be a little confusing. Would it be possible to let 'define-public*' replace 'define-public'? I don't really have an opinion on whether package-[field-]location should return the location of the 'let' form or the location of the 'define-public' form. I think 'package-location' should return the location of the 'let' form (or a surrounding form), because the 'commit' and 'version' variable from the 'let' form are part of the package -- change them, and you'll get a different package. Greetings, Maxime
Hello, Maxime Devos <maximedevos@telenet.be> skribis: > Why not always let the location of a package be the location of the > surrounding define-public* form, instead of having two separate > locations? Letting the location of a package be the location of the > define-public* form (or 'let' form) seems more useful to people using > "guix edit minetest-etheral" for example, and the package-field-location > code can easily be adjusted to support 'define-public*' (or let) forms. > > If two separate package-definition-location and package-location are > introduced, what should "guix show minetest-ethereal" show? The location > of the 'package' form, the location of the 'let' form or the location > of the 'define-public' form? A package always has a ‘location’, but it may lack a definition location, for instance if it’s produced by a procedure, or if it’s not bound to a top-level variable. Things like ‘package-field-location’ are likely more accurate if they start searching from the beginning of the (package …) sexp. These patches leave the UIs unchanged (‘guix show’, ‘guix edit’, etc.) because I think ‘location’ is good for these. > Having two separate define-public* and define-public macros might be a > little confusing. Would it be possible to let 'define-public*' replace > 'define-public'? ‘define-public*’ is exported as ‘define-public’, so package definitions do not need to be changed: #:replace ((define-public* . define-public)) > I don't really have an opinion on whether package-[field-]location should > return the location of the 'let' form or the location of the 'define-public' > form. I think 'package-location' should return the location of the 'let' > form (or a surrounding form), because the 'commit' and 'version' variable > from the 'let' form are part of the package -- change them, and you'll > get a different package. Yeah, I see what you mean. The work ‘guix refresh -u’ and ‘guix style’ do is essentially correlating live objects (package records) to their source code. This is necessarily an approximation; it’s similar to version strings constructed with ‘string-append’: that’s something that inspection of the live object cannot reveal, so we use heuristic to match common conventions. Thoughts? Thanks, Ludo’.
Hi Sarah, Sarah Morgensen <iskarian@mgsn.dev> skribis: > However... it doesn't work for unexported packages. It looks there are > about 200 such packages: > > ~/guix$ rg -U '\(define [^\(]+\n.*?\(package' gnu/packages --count --no-filename | awk '{a+=$1} END {print a}' > 233 Ah, hmm, well. I’d have said these are beyond our scope :-), and in fact we’d need to know how many among these 233 packages use the (let ((commit …)) …) idiom, but if this is deemed important, we can replace ‘define’ similarly. > And, to play the pessimist: > > What do we get out of this that couldn't be done by "go to package > location; read backwards one sexp until we reach a defining form" > (like Emacs' 'beginning-of-defun')? Nothing! It’s just easier to implement and more accurate—we’re sure to get the exact location of the ‘define-public’ form that surrounds the package record we’re looking at. Now, longer-term, I’d like to have Emacs/paredit-like features and more tools to correlate source and live objects. I found myself doing a bit of that in ‘guix style’, and I think that’s a fun area to explore so we can improve our package maintenance tools. Thanks, Ludo’.
Hello! Following our discussion on IRC, I pushed this as 8531997d2a1e10d574a6e9ab70bc86ade6af4733. I made one change, which is that the ‘definition-location’ field is now stored as a fixnum (column << 22 | line) rather than a vector. This should be enough to unlock Sarah’s patches at <https://issues.guix.gnu.org/50072>! Thanks, Ludo’.
From bc2d7144bb9ef0ea74f9ef5922d568291818de32 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Tue, 7 Sep 2021 21:19:11 +0200
Subject: [PATCH 2/2] packages: Add 'package-definition-location'.
Suggested by Maxime Devos <maximedevos@telenet.be>.
* guix/packages.scm (current-definition-location-vector): New syntax parameter.
(define-public*): New macro.
(<package>)[definition-location]: New field.
(package-definition-location): New procedure.
* tests/packages.scm ("package-definition-location"): New test.
---
guix/packages.scm | 42 +++++++++++++++++++++++++++++++++++++++++-
tests/packages.scm | 11 +++++++++++
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/guix/packages.scm b/guix/packages.scm
index 01de50ebd7..2f70ec9c64 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -52,6 +52,7 @@
#:re-export (%current-system
%current-target-system
search-path-specification) ;for convenience
+ #:replace ((define-public* . define-public))
#:export (content-hash
content-hash?
content-hash-algorithm
@@ -99,6 +100,7 @@
package-supported-systems
package-properties
package-location
+ package-definition-location
hidden-package
hidden-package?
package-superseded
@@ -385,6 +387,31 @@ one-indexed line numbers."
(location-line loc)
(location-column loc)))))
+(define-syntax-parameter current-definition-location-vector
+ ;; Location of the encompassing 'define-public'.
+ (const #f))
+
+(define-syntax define-public*
+ (lambda (s)
+ "Like 'define-public' but set 'current-definition-location' for the
+lexical scope of its body."
+ (define location
+ (match (syntax-source s)
+ (#f #f)
+ (properties
+ (let ((line (assq-ref properties 'line))
+ (column (assq-ref properties 'column)))
+ ;; Don't repeat the file name since it's redundant with 'location'.
+ (and line column
+ #`#(#,(+ 1 line) #,column))))))
+
+ (syntax-case s ()
+ ((_ prototype body ...)
+ #`(define-public prototype
+ (syntax-parameterize ((current-definition-location-vector
+ (lambda (s) #,location)))
+ body ...))))))
+
;; A package.
(define-record-type* <package>
package make-package
@@ -430,7 +457,10 @@ one-indexed line numbers."
(location package-location-vector
(default (current-location-vector))
- (innate) (sanitize sanitize-location)))
+ (innate) (sanitize sanitize-location))
+ (definition-location package-definition-location-vector
+ (default (current-definition-location-vector))
+ (innate)))
(set-record-type-printer! <package>
(lambda (package port)
@@ -455,6 +485,16 @@ it is not known."
(#f #f)
(#(file line column) (location file line column))))
+(define (package-definition-location package)
+ "Like 'package-location', but return the location of the definition
+itself--i.e., that of the enclosing 'define-public' form, if any, or #f."
+ (match (package-definition-location-vector package)
+ (#f #f)
+ (#(line column)
+ (match (package-location-vector package)
+ (#f #f)
+ (#(file _ _) (location file line column))))))
+
(define-syntax-rule (package/inherit p overrides ...)
"Like (package (inherit P) OVERRIDES ...), except that the same
transformation is done to the package P's replacement, if any. P must be a bare
diff --git a/tests/packages.scm b/tests/packages.scm
index 2a290bc353..3756877270 100644
--- a/tests/packages.scm
+++ b/tests/packages.scm
@@ -236,6 +236,17 @@
(eq? item new)))
(null? (manifest-transaction-remove tx)))))))
+(test-assert "package-definition-location"
+ (let ((location (package-location hello))
+ (definition (package-definition-location hello)))
+ ;; Check for the usual layout of (define-public hello (package ...)).
+ (and (string=? (location-file location)
+ (location-file definition))
+ (= 0 (location-column definition))
+ (= 2 (location-column location))
+ (= (location-line definition)
+ (- (location-line location) 1)))))
+
(test-assert "package-field-location"
(let ()
(define (goto port line column)
--
2.33.0