[asdf-devel] patch system-source-file

[Robert Goldman]

Greg Pfeil wrote:
> On 9 Jul 2009, at 13:08, Robert Goldman wrote:
> 
>> Greg Pfeil wrote:
>>> Here are a couple changes to ASDF that I made in the process of creating
>>> an ASDF browser for CCL's IDE:
>>>
>>>  system-source-file now works for systems without their own .asd
>>>  optional parts of systems (version, maintainer, etc.) don't leave
>>> their slots unbound
> 
>> The first of these looks good, but I'm less fond of the second change.
>> I've been bitten repeatedly by bugs caused by initforms that caused
>> slots not explicitly set to have some value that hides a mistaken
>> failure to fill the slot.
> 
> Yeah, I should have sent these as separate patches. The first one is
> more important, IMO, and the second is definitely debatable.

How about sending that first patch while we work through the second
issue.  Even if we add the initforms, seems like it's still worth
discussing the tradeoff b/w "" and NIL as defaults.  I'd be inclined to
prefer the latter, even at the expense of :type (or null string), so
that an unsupplied value is readily distinguishable from an empty value.

The argument for slot-unbound is that it makes an error when you think
you have set a slot, but you haven't.  For example, let's say I misspell
an initarg so that the value quietly vanishes.  Then I'd rather the
system hurl an error instead of quietly going off and doing something I
don't expect.
> 
>> I'd rather have us handle slot-unbound on those optional parts of the
>> system instead of stuffing a bunch of NILs in there.
> 
> When you say "us", do you mean implementing the accessors explicitly in
> ASDF to handle the condition, or having the caller handle/avoid the
> condition?

The latter.
> 
>> If one is expecting strings here one must still handle checking for NIL,
>> so having to check for slot-boundp doesn't seem that much more onerous.
> 
> Checking slot-boundp has a number of problems:
> 
>  you have to export the slot names

Is this really an issue?  Would someone outside the ASDF package really
be looking into these things?  I suppose possibly so...

>  it breaks the accessor abstraction because the slot names don't match
> the accessor names
>     (when (slot-boundp foo 'asdf::description) (system-description foo))

(handler-case ....
  (slot-unbound () ...)

avoids this problem, if you know that you want to quash unbound slots.

>  it breaks the accessor abstraction again because not everything that
> follows the accessor pattern has a slot
>     (when (slot-boundp foo 'asdf::pathname) (component-pathname foo))

See above.  If you just handle that error, then you don't need access to
the slot names.

> 
> The default value could just as well be "" as NIL. I chose NIL because
> you can kind of distinguish a system explicitly having "" as the value
> versus the case where the slot isn't set. If the distinction isn't
> worthwhile (which it probably isn't in this case, "" may be just as good.

It's possible that these slots are optional enough that I'm being
over-cautious here.  As I said, this is a reaction born of hard to find
bugs, probably mostly on attributes more important than these.

If it were easier to catch this kind of error when the system is
defined, that would be better, and the initforms would be benign.

So I'd suggest either:

1.  We reject my concern, add NIL initforms, and add (or null string)
type declarations (as documentation, more than anything else).

2.  We could leave the slots unbound, or default to nil, and add
accessors that use the (<accessor> object &optional error-p) pattern so
that people who expect these slots to be set can see errors.

Best,
r