record-instance? from (chicken memory representation) fails in modules

[Kooda]

Since 5.0.0, we prefix record symbols with the module name, this makes the behavior of record-instance? very strange, as the same call to it can work outside a module but fail inside.

For example, this breaks the record matching of the matchable egg.

Here is a test case:

(module foo ()
(import scheme)
(cond-expand
  (chicken-4 (import chicken) (use lolevel))
  (chicken-5 (import (chicken base) (chicken memory representation))))
(define-record bar)
(assert (record-instance? (make-bar) 'bar)))

[sjamaan]

It seems to me we have several ways to do this.

We can document this, so that the user will know to pass in 'foo#bar as the record name.

We could also strip the prefix when comparing, so that any bar is accepted. That means it will also accept a bar from a completely different module, which might not be desirable (but it's more consistent with the pre-5 version).

Finally, we could make it syntax somehow (or add a syntax for getting a namespaced record name) and read out ##sys#current-module.

I prefer simply documenting the current behaviour, because (chicken memory representation) is a relatively low-level module already anyway.

[felix winkelmann]

As beeing discussed on IRC there is also a problem with record-variants/coops: here structure tags are taken literally instead of the variable holding the module-decorated tag name.

Attached is a patch that modifies record-variants to use the tag variable implicitly created for a record instead of treating the record name directly as the tag (which will fail with modules). This patch also fixes some minor problems. I post this here since a few issues may be worth noting:

• Jim should check whether my modifications of record-variants makes sense
• define-record-variant will not work for a nonexistent record definition

The latter case appears in coops: here we use a record-variant direclty, instead of shadowing an existing record definition, this is, strictly speaking, a hack. Now, define-record-variant access the record-tag variable, which will of course no exist, so I've added a dummy record definition for coops-instance. The other coops fixes should straightforward.

[felix winkelmann]

Patches for coops, record-variants

[felix winkelmann]

Some nore notes:

We should document for record-instance? that access to the tag variable is needed and suggest that it will be exported from modules.

We should check the eggs that define records for exporting the tag variables as well.

[sjamaan]

Matchable has been changed to use the unquoted identifier in trunk. This may be tagged once we have a new Salmonella run after the failures due to our keywords CR have been resolved.

[Jim Ursetto]

I will take a look. Sorry, I have been on vacation for the last 8 days, just got back.

[Jim Ursetto]

I approve of the record-variants patch. I thought this sounded familiar, and found a patch email from Peter from 2017 July 14 entitled "Fix record type tags to be nonglobal by module-prefixing them". It notes that record-variants and coops need to be fixed after the patch (54b0d5adcc855), and describes this solution exactly. It just appears we never followed up.

"A long time ago, we had a patch that tried to prefix the current module
name onto a record type's tag (727b2b3fea2714).
This was reverted later on because it turned out to be somewhat
incompatible with various things, like the record-variants egg.

[...] I believe record-variants can be fixed if we also fix #1342 by
defining the "type name" which you pass to define-record-type as an identifier.

[...] I'd add a patch for record-variants, but I see we have no port to
CHICKEN 5 for it yet. In any case, instead of quoting the type name, it would
simply be used unquoted, as that's the identifier which holds the tag.
I don't know exactly what the problem with coops was, but I presume it was
either caused by record-variants not working or we can fix it in a similar
way by using the record identifier instead of the quoted tag."

[felix winkelmann]

I've applied both patches.