[Rd] Breaking Change in Rcomplex Layout?

[Tomas Kalibera]

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 using 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 using 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
>>         <https://bugs.r-project.org/show_bug.cgi?id=18430> that
>>         > relates to the layout of the Rcomplex typedef. Previously
>>         it was a struct,
>>         > but now it's a union by default
>>         >
>>         <https://github.com/r-devel/r-svn/commit/862f9f816ff3ff3cb3f851603f19e99f60a98475#diff-e9b09a44d9dc69444eca2300325e790a0cc6d2c8c3960f47519c7f8ef896f9e4>,
>>         > 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 using r-project.org mailing list
>>         > https://stat.ethz.ch/mailman/listinfo/r-devel
>>
	[[alternative HTML version deleted]]