Breaking Change in Rcomplex Layout?
On 4/4/23 04:27, Michael Milton wrote:
Hi Tomas, Thanks for this explanation. As you can probably tell I'm not much of a C person, so I didn't realise this change would be invisible to C users. I suppose R's stability contract only applies to C, and therefore changes that break other languages such as Rust are out of scope. You are right that this error is confused by bindgen's?inability to handle anonymous types, but even without that it would be a breaking change in Rust because code that accesses r or c must now access the struct variant of the union and then access the struct field.
Which I believe can be rephrased as that it also matters how Rust can handle anonymous nested structures, or what is the right mapping of the involved C types to Rust. I can't comment on that and it is definitely out of scope of maintaining R, R just cares about its C interface. As long as that is done right, correct mapping from C to other languages should work as well.
Also, this code has to now be in an unsafe block because of the potential to read from the wrong variant <https://doc.rust-lang.org/reference/items/unions.html#reading-and-writing-union-fields>. I guess we will have to handle this using some kind of compile-time abstraction that handles this type differently depending on the R version being used.
That's a Rust thing, but it is certainly worth making sure you don't re-introduce the problem that R had before this change, on your end. Tomas
Cheers
On Tue, Apr 4, 2023 at 12:15?AM Tomas Kalibera
<tomas.kalibera at gmail.com> wrote:
On 4/3/23 15:50, Michael Milton wrote:
Okay, but I'm afraid this will only mean something to Rust users.
The reason being that we encountered this issue in extendr: a
Rust extension library for R. The specific compiler errors we
encounter happen because bindgen (the Rust code generation
library) read the changed R header files, and generated a new
type definition for Rcomplex. Then, our downstream code that uses
that bindgen-generated code caused rustc compiler errors such as:
|error[E0560]: union `libR_sys::Rcomplex` has no field named `r`
--> extendr-api\src\robj\into_robj.rs:93:20 | 93 | Rcomplex { r:
0., i: 0. } | ^ `libR_sys::Rcomplex` does not have this field | =
note: available fields are: `__bindgen_anon_1`, `private_data_c`|
and
|error[E0609]: no field `i` on type `libR_sys::Rcomplex` -->
extendr-api\src\scalar\rcplx_default.rs:105:35 | 105 |
Rcplx(c64::new(val.r, val.i)) | ^ unknown field | = note:
available fields are: `__bindgen_anon_1`, `private_data_c` help:
one of the expressions' fields has a field of the same name | 105
| Rcplx(c64::new(val.r, val.__bindgen_anon_1.i)) | +++++++++++++++++|
That seems like the generator does not support anonymous
structures. Thanks for letting me know, but I am afraid it is not
something that could be fixed on the R end.
However, to put this into context, I would expect that C, C++
packages would encounter a similar issue if they tried to access
or modify specific Rcomplex?fields in the same way.
No, you can access the fields as before in C (C11):
Rcomplex z;
z.r = 1; z.i = 2
this is what anonymous structures allow, this is why the structure
is anonymous, not to break existing code.? The union there is to
tell the compiler about the aliasing, to prevent it from
misoptimizing the code. As far as I can tell this is valid C code,
not a breaking change.
The only issues I know about so far are that you get a warning for
initializations such as "z = {1, 2}", but still, they will be
interpreted correctly by the compiler within the standard.
However, "z = { .r = 1, .i = 2}" worked before and works with the
change.
The definition unfortunately is not valid C++, but works with C++
language extensions supported at least by GCC and clang, yet you
may see warnings. R itself doesn't use C++, but some packages do.
Please note your subject is not quite right: the layout of
Rcomplex did not change, at least not on systems supported by R.
The layout is the same on systems which use self-alignment of
structures.
Also please keep in mind R-devel is meant for unstable changes, it
may not work, things may be reverted, it is partially used for
testing. I've still tested this on all CRAN and Bioconductor
packages before committing even to R-devel, but this is not always
done and cannot be due to how much resources it takes. Obviously
this will get a NEWS entry when/if it gets ported to 4.3.0 branch,
in the form it would have at that point.
Best
Tomas
On Mon, Apr 3, 2023 at 11:39?PM Tomas Kalibera
<tomas.kalibera at gmail.com> wrote:
On 4/3/23 14:07, Michael Milton wrote:
> Hi all,
>
> There seems to have been a breaking change in the R trunk
caused by a fix
> to bug 18430
> relates to the layout of the Rcomplex typedef. Previously
it was a struct,
> but now it's a union by default
>
> which breaks downstream code that relied on this layout.
I'm aware of
> the R_LEGACY_RCOMPLEX variable, but I still wouldn't expect
an unreported
> breaking change especially if it's aimed at R 4.3 (although
I'm not sure if
> it is). I believe src/include/R_ext/Complex.h, which this
patch affects, is
> considered part of the public R ABI since it's included by R.h.
>
> What should I, as a downstream package developer, do about
this change?
Please report the actual problem you have ran into.
Thanks
Tomas
>
> Cheers.
>
>? ? ? ?[[alternative HTML version deleted]]
>
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel