[Rd] Breaking Change in Rcomplex Layout?

[Michael Milton]

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

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