Summary

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

Metadata

Attachments

Description

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)))

Changes and comments

[2019-02-02 13:03:29 UTC] sjamaan wrote:

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.

[2019-04-23 13:45:55 UTC] felix wrote:

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:

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.

[2019-04-23 13:46:24 UTC] felix attached record-tag-fun.diff (description=Patches for coops, record-variants)

[2019-04-23 14:02:27 UTC] felix wrote:

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.

[2019-05-25 10:54:21 UTC] sjamaan wrote:

Matchable has been changed in r37616 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.

[2019-05-25 10:54:21 UTC] sjamaan wrote:

1558774488896262

[2019-06-05 22:30:06 UTC] zbigniew wrote:

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

[2019-06-05 23:37:12 UTC] zbigniew wrote:

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."

[2019-06-06 10:07:05 UTC] felix wrote:

I've applied both patches.

[2019-06-06 10:07:05 UTC] felix changed status from new to closed

[2019-06-06 10:07:05 UTC] felix set resolution to fixed