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
>> <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 at r-project.org mailing list
>> > https://stat.ethz.ch/mailman/listinfo/r-devel
>>
[[alternative HTML version deleted]]