Only check the label, not the label/pl, for duplicate imports

[dpk]

Fixes #929.

I am not yet certain that this patch is complete. At the very least, this demonstrates that the behaviour of importing identifier properties appears to work as expected:

(library (foo)
  (export floop floop-val)
  (import (chezscheme))
  (define floop)
  (define-syntax floop-val
    (lambda (stx)
      (lambda (lookup)
        (syntax-case stx ()
          ((_ id) #`'#,(lookup #'id #'floop))))))
  (define-property floop floop #t))

(library (bar)
  (export floop bart)
  (import (foo) (chezscheme))
  (define-syntax bart
    (lambda (stx)
      (lambda (lookup)
        (syntax-case stx ()
          ((_ id) #`'#,(lookup #'id #'bart))))))
  (define-property floop bart 12))

(library (baz)
  (export barted flooped)
  (import (foo) (bar) (chezscheme))
  (define barted (bart floop))
  (define flooped (floop-val floop)))

(library (baz2)
  (export barted flooped)
  (import (foo) (only (bar) bart) (chezscheme))
  (define barted (bart floop))
  (define flooped (floop-val floop)))

(eval 'barted (environment '(baz)))   ;=> 12
(eval 'flooped (environment '(baz)))  ;=> #t

(eval 'barted (environment '(baz2)))  ;=> #f
(eval 'flooped (environment '(baz2))) ;=> #t

However, I am not sure if label can contain a label/pl as well as the entry-label. Moreover, the Right Thing is probably to still signal a duplicate if an imported identifier has a conflicting property definition, but this patch won’t do that. (Then again, this matches the behaviour that it also won’t stop you defining the same property multiple times in a body.)

It may also be that there is a better place to fix this.

There should likely also be a test for this; the above example could serve as a basis.

I would appreciate feedback from those familiar with the innards of syntax.ss.

[dpk]

Apologies for the bump, but I would appreciate an initial review of this.

The problem is that when you use any kind of qualified import (only, except, rename, etc.) of a library with identifier properties, a label/pl ends up in the entry-label slot when checking for duplicate imports. Since the duplicate check was with eq?, it would always falsely report a duplicate. This patch modifies the check for a duplicate by converting a label/pl to a label before doing the check.

The question is whether it might be better to change it to avoid putting a label/pl in an entry-label slot in the first place. That might be a somewhat more invasive patch, although ultimately cleaner: I’m just not sure enough of the data flow through syntax.ss to make that change with certainty at the moment.

If someone could confirm I’m on the right track or suggest a better way to fix this issue, I’ll add some tests and mark this ready to merge.

[mflatt]

I doubt that I'm the right person to comment here, but instead of this change, I would be inclined to change the record-id! call around 3329

(record-id! defn-table id (resolved-id->label/pl id))

to use resolved-id->label :

(record-id! defn-table id (resolved-id->label id))

That would be more in line with the formal argument name label, with other record-id! calls that provide a label (not a label/pl), and with the way that record-iface! (used for an iimport without renaming) calls check-and-record! with (resolved-id->label id).

The discussion in PR #929 notes that when two imported modules provide different values for a given binding—property combination, the later import wins. That seems consistent with the way that (define-property x cons ....) can be used twice in the same definition content, where the later one wins for references that follow both definitions. A lot more checking would be needed in general, I think, to disallow conflicts like that.