Allow fallback method of storing properties for "forbidden" names

[dgkf]

I'm not sure if I've just had bad luck or if these forbidden properties names are just so useful that they pop up frequently, but this has been a prickly part of S7 for me in a couple different projects now. I've only tried to use S7 in earnest in two packages and both have butted heads with this constraint in different ways.

I'm referring to these forbidden property names.

Just to illustrate a couple cases where this has come up in my exploration so far, I've encountered this during:

• implementing the debug adapter protocol, which has classes that have names fields. I'm trying to be faithful to the protocol, but I need to have a special case for this one field because this is a forbidden property name.
• implementing a specialized version of a function that does return type checking, I store the return type as a class property, but again hit this constraint. Here I have more control over the naming convention, but was surprised to see that there are these limits.

I'd like to propose a couple alternative designs

1. The simplest solution would be to store properties in a attr(obj, "properties") list to avoid any constraints on the names. However, I assume that this was consciously avoided because of the extra lookup.
2. If a list is undesirable, perhaps introducing it only as a backup option for these specific elements so that nearly all properties remain surface-level attributes, but a few select forbidden names get relegated to an internal list. During property getting and setting, there would be branching logic based on whether the property name is forbidden or not.

[hadley]

IMO that would force S7 objects further away from S3 objects, which would require serious thought. IMO it would be easier on the whole to just document a convention like .names if you want to use one of the eight forbidden names.

[dgkf]

IMO that would force S7 objects further away from S3 objects

Ah, I didn't realize object structure parity was a design goal. If so, that makes sense to inherit the constraint.

which would require serious thought.

What would a next step look like? I don't mind putting together a draft PR to start sparking the discussion. Or would you prefer to hash out more of a plan first?

just document a convention like .names if you want to use one of the eight forbidden names.

Yeah, that's what I've done so far. It's the obvious way to sidestep the problem, but it's definitely not a satisfying solution.

[t-kalinowski]

Since we generally have a unique SEXP OBJSXP type, I wonder if we can relax some of the checks in Rf_setAttrib to permit setting (some of?) those attrib names on an OBJSXP?

For reference, these are the currently forbiden names:
"names", "dim", "dimnames", "class", "tsp", "comment", "row.names", "..."

[t-kalinowski]

Another approach might be to mangle the forbidden names. E.g, make these the new "forbidden" names:

".names", ".dim", ".dimnames", ".class", ".tsp", ".comment", ".row.names", "...."

Then automatically resolve, e.g., x@names to attr(x, ".names"). Perhaps with more mangling like .__S7_prop__names__.

[dgkf]

Since we generally have a unique SEXP OBJSXP type, I wonder if we can relax some of the checks in Rf_setAttrib to permit setting (some of?) those attrib names on an OBJSXP?

Just to check my understanding here, do you mean to limit the forbidden names to only those attributes that are used by a particular parent class? e.g. for a new_class(parent = class_character), restrict names, but for a new_class(parent = S7_object), avoid restrictions?

Maybe one other consideration is whether attributes for S7-ified atomic classes and base classes should expose these attributes as properties to be more consistent? For example,

S7::class_character
#> <S7_base_class>: <character>
#> @properties:
#>  $ names: <NULL> or <character>

Instead of restricting properties when they clash with base attributes, maybe the more holistic solution is to incorporate base attributes as properties.

[lawremi]

I've always been a fan of the proposal for @() to retrieve attributes from non-S7 objects, in which case they would be syntactically similar to properties, but not semantically (no getter/setter, validation, etc).

One rationale for these attributes being reserved is that they are so ingrained in the R ecosystem. Anything defining a property with a name like names or dim would very much want to adhere to the established semantics, which is one reason why they are hardcoded and restricted at the C level. There are of course many S3/S4 examples of defining custom versions of base types, like vectors (S4Vectors) and matrices (Matrix). In such cases, munging the name, as already suggested, was the pragmatic solution.

We could indeed generalize setAttrib(), as @t-kalinowski suggests. For example, @names would dispatch to length() to validate the length of the input, in addition to the other checks.

@t-kalinowski had indicated interest in restarting his work on S7; if that's still the case, we should start discussing general next steps.

[hadley]

@dgkf as a general philosophical point, I'd argue that this is an example of Chesterton's fence and thus if you want us to change it, it's your responsibility to figure out why it exists.

Overall, my sense is that our collective time is better spent fixing clear bugs and adding new features, rather than trying to reduce this very small inconvenience.

[dgkf]

@hadley I'm not sure if this was intended to be as dismissive as I'm reading it, but it's coming across a bit harsh. I see this ticket as a place to document a pain point and spark a discussion while it's front-of-mind -- that's it. And to that end, I think its brought together some fruitful and constructive thoughts on the topic.

I am in a unique position where this is probably worth my time to try to fix. I'm trying to programmatically build ~200 classes from the debug adapter's machine-readable spec, which includes objects that have fields with these names. I can either invest my time maintaining special cases at each stage of the implementation or contribute a fix to solve the problem at its root -- I think both paths probably require similar investment on my part, but my preference is to simultaneously improve a small inconvenience in S7 while lowering the maintenance overhead for a package.

I appreciate that it's not just my time needed for design and review, which is why I started by asking what would be a preferred next step.

From the discussion so far, it seems like there's been a bit of consensus around mangling/munging these names and I'd be happy to open the PR to add the change.

[t-kalinowski]

@lawremi I agree, let’s plan to pick this up after the new year!

[t-kalinowski]

After some thought, I’m starting to think we should accept the “forbidden” property names in S7, and support this by mangling the names behind the scenes so they can exist safely as R attributes.

I don’t think we’ll ever want x@names to act as an alias for names(x), and the same applies to dim(), class(), and the other base attributes. But I can see cases where an S7 class might reasonably have its own @class or @comment property without implying any connection to the base semantics.

The current restriction exists only because S7 properties are stored as R attributes. That’s an implementation detail rather than a semantic statement about what property names should be allowed. If we mangle the underlying attribute names internally, we can preserve the intended semantics while removing an unnecessary limitation for users.

[JosiahParry]

I just encountered this issue and was going to mark it as a regression in the package but then found this.

There's not any mention in the NEWS.md of reserved names for S7.

Re-rendering my blog post https://josiahparry.com/posts/2024-11-21-s7-options-objects leads to

class_readr_opts <- new_class(
  "readr_opts",
  properties = list(
    locale = .locale,
    na = .na,
    quote = .quote,
    comment = .comment,
    trim_ws = .trim_ws,
    skip = .skip,
    n_max = .n_max,
    guess_max = .guess_max,
    name_repair = .name_repair,
    num_threads = .num_threads,
    progress = .progress,
    show_col_types = .show_col_types,
    skip_empty_rows = .skip_empty_rows,
    lazy = .lazy
  )
)

opts <- class_readr_opts()
#> Error in `new_class()`:
#> ! property can't be named: comment

I think it's okay if there are reserved names, having them documented with why would be wonderful. I don't understand the OBJSXP enough to understand why these wouldn't be permitted. Is this due to the attributes being reserved—like row.names in a data.frame or matrix, for example?