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