Restore compatibility with release ggplot2 - #753
t-kalinowski wants to merge 11 commits into
Conversation
|
I came across this compatibility problem today (#747), just wanted to chime in and say that this fix doesn't seem to fully resolve the issue (at least, not on the macOS and Ubuntu machines I've tried it on today). pak::pak("ggplot2")
pak::pak("RConsortium/S7@fix/ggplot2")
library("S7")
# Can load the latest release of ggplot2
library("ggplot2")
# `theme()` works, as reported
theme()
#> <theme> Named list()
#> - attr(*, "class")= chr "gg"
#> @ complete: logi FALSE
#> @ validate: logi TRUE
# Can't create a ggplot object
ggplot()
#> Error in `class_ggplot()`:
#> ! <ggplot2::ggplot> object properties are invalid:
#> - @scales must be S3<ScalesList>, not S3<ScalesList/ggproto/gg>
#> - @guides must be S3<Guides>, not S3<Guides/ggproto/gg>
#> - @coordinates must be S3<Coord>, not S3<CoordCartesian/Coord/ggproto/gg>
#> - @facet must be S3<Facet>, not S3<FacetNull/Facet/ggproto/gg>
#> - @layout must be S3<Layout>, not S3<Layout/ggproto/gg>Created on 2026-08-26 with reprex v2.1.1
|
hadley
left a comment
hadley
left a comment
There was a problem hiding this comment.
Since this is at least the second time we've accidentally broken ggplot2, maybe it's worth a new GitHub action that tests ggplot2 explicitly? (I think it's fine to do just on CI, since a test will be fiddly/expensive and required ggplot2 as a suggested dep)
|
|
||
| # helpers ----------------------------------------------------------------- | ||
|
|
||
| # Does `child`'s S3 dispatch inherit from `parent`'s? S3 systems may prepend |
There was a problem hiding this comment.
I'm surprised that this doesn't explicitly mention the backwards compatibility issue.
There was a problem hiding this comment.
Added a comment mentioning the backwards compatibility motivation. This should make it easier to remove in the future.
| # Detect the stub constructor that `new_S3_class()` inserted before abstract | ||
| # S3 classes had explicit metadata. Needed for class definitions serialized by | ||
| # older versions of S7. | ||
| is_S3_stub_constructor <- function(constructor) { |
There was a problem hiding this comment.
I wonder if it would be better to add an explicit _version attribute to classes so we could make it clear when we're adding backward compatibility shims?
There was a problem hiding this comment.
Added a _version attribute, starting at 1L. I used an integer representation version rather than the S7 package version because it only needs to change when the representation changes. Older class definitions remain unversioned, and the backwards compatibility code only runs for those definitions.
New S3 class definitions now carry a representation version. Limit legacy abstract-class detection to unversioned definitions while retaining the older inheritance match needed by ggplot2.
Add a release-R job that builds and prints a ggplot2 plot on a temporary PNG device without adding ggplot2 to Suggests.
|
Added a separate CI job that installs ggplot2 alongside the local S7 checkout and prints a plot to a temporary PNG device. This exercises the actual drawing path without adding ggplot2 to Suggests. |
|
@jamesotto852 Thank you for the comment! This should be fixed now, if you want to try again. |
| upload-snapshots: true | ||
| build_args: 'c("--no-manual","--compact-vignettes=gs+qpdf")' | ||
|
|
||
| ggplot2: |
There was a problem hiding this comment.
I'd put this in a completely separate job so it doesn't get clobbered when you run use_tidy_github_actions()
There was a problem hiding this comment.
Moved the ggplot2 compatibility check to its own test-ggplot2.yaml workflow, so use_tidy_github_actions() can update R-CMD-check.yaml without removing it.
Keep R-CMD-check focused on package checks and run the ggplot2 compatibility test in test-ggplot2.yaml on pushes and pull requests.
| } else if (is_S3_class(class)) { | ||
| class$abstract %||% is_default_constructor(class$constructor) | ||
| class$abstract %||% | ||
| (is.null(attr(class, "_version", exact = TRUE)) && |
There was a problem hiding this comment.
I wonder if the fallback should live in is_default_constructor() for clarity?
| any = TRUE, | ||
| S4 = methods::is(x, what), | ||
| S7 = has_S7_class(x) && inherits(x, S7_class_name(what)), | ||
| S7 = inherits(x, "S7_object") && inherits(x, S7_class_name(what)), |
There was a problem hiding this comment.
Could this fallback live in has_S7_class() or is that too expensive?
| # downcasts. | ||
| # S7 wrappers of base/S3 types append "S7_object", which we ignore. | ||
| class_dispatch_inherits <- function(parent, child) { | ||
| parent <- drop_S7_object(parent) |
There was a problem hiding this comment.
Only use this path if version is NULL?
3 participants
Fixes #747.
Release ggplot2 packages can again load with the development version of S7. Their stored
S3<gg>class definitions were created before S7 recorded explicit abstract metadata or tagged generated constructors, so S7 treated the abstract parent as concrete and rejected the list used to construct a theme.Internally, this restores the legacy stub-constructor check only for S3 class definitions that lack explicit abstract metadata. Current definitions continue to use the metadata and constructor tag, and concrete S3 parents still receive the usual parent validation. The regression fixture reproduces the S7 0.2.2 class shape.
Testing
devtools::test()on R 4.4.3: 1,288 tests passed.devtools::check()on R 4.4.3: 0 errors, 0 warnings, and 4 unrelated notes.theme()with each.