Introduce a path! macro for creating items from simple paths

[afetisov]

This PR introduces the path! macro which can be used like

let p: syn::Path = path![::core::ptr::null_mut];
let e: syn::Expr = path![Vec::new];
let t: syn::Type = path![::libc::c_char];

It can output any type T which has impl Make<T> for Path. Currently this means Path, Type and Expr. Currently the macro supports only simple raw paths, i.e. no variable interpolation (only literal identifiers), no qualified self and no generic parameters. The restriction on simple interpolation of variables may be lifted in a later PR. The restriction on qualified self and generics will likely no be lifted, since they seem far beyond the capabilities of reasonable declarative macros.

This PR also adds impl Make<Path> for slices and arrays. This allows to avoid unnecessary allocations when creating paths (almost all paths created by the transpiler have a statically known size). As an extra benefit, switching from vec![..] to [..] construction looks more lightweight, and allows processing of the array by rustfmt (vec! calls are not formatted properly in general).

This PR also specializes the methods of Builder with respect to most generic parameters. Those generics were redundant since each position could be occupied only by a single specific type. E.g. Make<Box<Expr>> is implemented only by Box<Expr>, so that is always the type of impl Make<Box<Expr>> arguments.

Specializing those generic parameters has no restriction on the practical usability of the API, and it in fact makes it more flexible: generic parameters break type inference. If I have fn foo<T: Trait>(t: T) and fn bar<R>() -> R, then foo(bar()) will result in a compile error since the type of R is generally impossible to infer. The type inference is heavily used by the path! macro to overload on the return type.

As a bonus, specializing the parameters allows to remove a significant number of redundant explicit type casts. It also results in better compile speed (generics aren't fully compiled in the defining crate, and are instantiated separately in each codegen unit). The speed benefit is minor, but a nice touch.

[kkysen]

Just commenting so it's easier to find.

[kkysen]

Thanks! This looks really great and really simplifies things.

A couple of questions from a quick overview, though I'll look more closely tomorrow:

• Why path![] instead of path!()?
• In path![Vec::new], how and what does Vec resolve to?
• Can this create PathSegments that can then be joined to generics so we can at least use path! for the stem of those paths?
• Are the paths in path![] IDE clickable? I think that's somewhat important.
• When you saying qualified self, do you mean self or Self?
• In the common case, can it default to absolute paths and check that that path is correct at compile time?
• Does this fix Qualified paths like libc::c_int should be globally qualified like ::libc::c_int to avoid name clashes #447? I saw it does at least most of it I think.

I agree that removing the unnecessary generics is good. Better compile speed and no ugly casts due to no inference, as you said. Removing lots of unnecessary vec![]s is also great, as most of those should've been arrays, slices, or iterators.

[kkysen]

Also, run ./scripts/test_translator.py tests/ first. There are errors in CI I think related to missing derive macros, like c2rust_bitfields::BitfieldStruct.

[afetisov]

Why path![] instead of path!()?

Because I refactored a lot of expressions of the form mk().path(vec![ .. ]), and it was much less work to keep the brackets :)

I don't think there is any strong argument for any of the bracket types, so that reason is as good as any. If you want, you can conceptualize it as an array of path segments, separated by :: instead of ,.

Braces would look a bit heavyweight, I associate them with complex code blocks and not simple paths. Parentheses are possible, but that would be a ton of work for little gain. Personally, I feel that parentheses kinda merge with the contained path, and not stand out enough. They will blend even more if the macro is ever modified to accept optional function call arguments. Brackets are unlikely to be used inside of that macro.

In path![Vec::new], how and what does Vec resolve to?

Nothing, it's just a token. It is essentially the same as with sequences of strings that we had before, but it is easier to type, easier to read, and more greppable (i.e. I can easily search the code for all uses of Vec::new both in the transpiler and in the transpiled code).

I doubt that it's possible to make Vec resolve to anything. Even if I could make it, you could ask "what does new resolve to?", and there just isn't any sensible answer. Special-casing the first token doesn't seem like a useful approach (e.g. most paths would then resolve to core or std).

Can this create PathSegments that can then be joined to generics so we can at least use path! for the stem of those paths?

Maybe, but I would like to think more on the good API for that case. There are a lot of cases like mk().path(["core", "ptr", access]) which would benefit from your suggestion. Imho it would be best to translate that case via simple identifier interpolation: path![core::ptr::#access]. Then # token as interpolator is chosen by analogy with quote crate, which is a standard for quasiquotations in Rust. It's also very convenient in that it's almost never used, apart from attributes, which are easy to syntactically distinguish.

There are some more complex cases, but I currently have no opinion on the best way to support them (or even whether they should be supported by the macro in the first place).

Are the paths in path![] IDE clickable? I think that's somewhat important.

That would be great, but I have absolutely no idea how it would be possible to implement. Quite likely that it's just impossible. It is also somewhat of dubious value: it would mean that working on the transpiler would require having as direct dependencies all crates which may be used by the transpiled code. Ok, adding f128 or memoffset to Cargo.toml would be a bit confusing, but possible. But what if you are transpiling some platform-specific code (e.g. running on some embedded or a console), which requires crates that just can't be built on your host platform, or maybe not even available to you?

On the bright side, an explicit full path is pretty easy to search in the IDE. I don't know about VsCode, but CLion understands Rust paths in symbol search window.

When you saying qualified self, do you mean self or Self?

I mean that you cannot write paths like <char>::is_numeric or <Self as Trait>::method.

In the common case, can it default to absolute paths and check that that path is correct at compile time?

Absolute paths are no different from relative paths at the type level, they are both Path. I also consider it counterproductive: just write the specific path you want to have. It's good that you get what you see, and implicit absolutization may also lead to confusing errors in case of collision between crate names and module names.

Does this fix #447? I saw it does at least most of it I think.

No, although I patched some obvious issues. In particular, I didn't fix anything in the paths which I didn't move to the new API, and I didn't fully qualify imported macros.

[afetisov]

Also, run ./scripts/test_translator.py tests/ first. There are errors in CI I think related to missing derive macros, like c2rust_bitfields::BitfieldStruct.

Works on my machine 🤷‍♂️ Actually not really, but I get an entirely different and even more confusing set of errors: "asm.aarch64/asm-test" fails to link because each of its object files "is incompatible with elf_x86_64". Shouldn't it be doing a cross-compilation? I also get an "unused argument" error in one of the inline assembly snippets.

This is certainly not related to my PR. In fact, I have also been getting many weird errors when I ran the test script in the past, even on the 2020 version of c2rust. I just thought that it's some misconfiguration of my environment, or some known failure which is somewhy not ignored by my test runner.

I didn't change anything in the c2rust-bitfields crate, and I don't seem have changed anything related to bitfields in any suspicious way in the transpiler either, so I'm confused where the error is coming from.

FYI, I'm running tests on WSL 2.0 Ubuntu 20, with CLang 11.

[kkysen]

Because I refactored a lot of expressions of the form mk().path(vec![ .. ]), and it was much less work to keep the brackets :)

I don't think there is any strong argument for any of the bracket types, so that reason is as good as any. If you want, you can conceptualize it as an array of path segments, separated by :: instead of ,.

Braces would look a bit heavyweight, I associate them with complex code blocks and not simple paths. Parentheses are possible, but that would be a ton of work for little gain. Personally, I feel that parentheses kinda merge with the contained path, and not stand out enough. They will blend even more if the macro is ever modified to accept optional function call arguments. Brackets are unlikely to be used inside of that macro.

That's reasonable. I think there are possible extensions to this initial API that could work well with braces, so it may be fine.

Nothing, it's just a token. It is essentially the same as with sequences of strings that we had before, but it is easier to type, easier to read, and more greppable (i.e. I can easily search the code for all uses of Vec::new both in the transpiler and in the transpiled code).

I doubt that it's possible to make Vec resolve to anything. Even if I could make it, you could ask "what does new resolve to?", and there just isn't any sensible answer. Special-casing the first token doesn't seem like a useful approach (e.g. most paths would then resolve to core or std).

Ah, that makes sense, though we should avoid unqualified paths like Vec::new (because they could be shadowed). Don't need to fix that here, though. As for resolving, it is possible as I noted here: #461 (comment) (Playground).

pub trait TypeName {
    fn type_name() -> &'static str {
        std::any::type_name::<Self>()
    }
    
    fn type_name_of_val(&self) -> &'static str {
        Self::type_name()
    }
}

impl<T> TypeName for T {}

fn main() {
    let x = Vec::<()>::new.type_name_of_val();
    println!("{x}");
}

It works for types and functions, but as you can see it doesn't work well with generics, and doesn't work for macros and modules, so I'm not sure how useful it is, but it does have some benefits in giving the full, canonical path, seeing through re-exports like std to core/alloc.

Maybe, but I would like to think more on the good API for that case. There are a lot of cases like mk().path(["core", "ptr", access]) which would benefit from your suggestion. Imho it would be best to translate that case via simple identifier interpolation: path![core::ptr::#access]. Then # token as interpolator is chosen by analogy with quote crate, which is a standard for quasiquotations in Rust. It's also very convenient in that it's almost never used, apart from attributes, which are easy to syntactically distinguish.

There are some more complex cases, but I currently have no opinion on the best way to support them (or even whether they should be supported by the macro in the first place).

Re possible future interpolation, I think something like this would be preferable and easier to implement:

path![::core::ptr::null_mut].join(generic_args(&[ty]));
path![::core::ptr].join(access);

That would be great, but I have absolutely no idea how it would be possible to implement. Quite likely that it's just impossible. It is also somewhat of dubious value: it would mean that working on the transpiler would require having as direct dependencies all crates which may be used by the transpiled code. Ok, adding f128 or memoffset to Cargo.toml would be a bit confusing, but possible. But what if you are transpiling some platform-specific code (e.g. running on some embedded or a console), which requires crates that just can't be built on your host platform, or maybe not even available to you?

On the bright side, an explicit full path is pretty easy to search in the IDE. I don't know about VsCode, but CLion understands Rust paths in symbol search window.

With something like this,

macro_rules! path {
    ($path:path) => {
        {
            #[allow(unused_imports)]
            use $path;

            stringify!($path).split("::")
        }
    };
}

the path (a path, not a series of idents) is IDE-clickable. I didn't try yet with your version.

I still think something like this is quite useful, but it could be optional. Maybe a happy-path path! and an unchecked_path! if we don't want to add those dependencies directly. Still, we really only use f128, memoffset, and libc I think, and those are fine to include as dependencies, as c2rust_transpile is already quite large.

I mean that you cannot write paths like <char>::is_numeric or <Self as Trait>::method.

Ah. I don't think we ever use those, and hopefully, we don't need to. They're quite ugly.

Absolute paths are no different from relative paths at the type level, they are both Path. I also consider it counterproductive: just write the specific path you want to have. It's good that you get what you see, and implicit absolutization may also lead to confusing errors in case of collision between crate names and module names.

I think we want to make relative paths harder to use, as we should be using absolute paths in almost all of the cases. So I thought path! should default to adding the leading :: if it's not there, and a separate relative_path! or rel_path! that doesn't. We always want the crate name to take priority over module and other local item names, and that's exactly the problem we want to fix here and make it easier to do so.

No, although I patched some obvious issues. In particular, I didn't fix anything in the paths which I didn't move to the new API, and I didn't fully qualify imported macros.

Got it. We'll finish that up later then.

[kkysen]

Re the CI failures and testing, the local errors you are getting are likely because you've rustup target added arm-unknown-linux-gnueabihf and aarch64-unknown-linux-gnu. test_translator.py detects those and only runs the cross-compile tests if they are detected, but more than just those targets is needed (cross-compiler linkers and emulators), so it fails. If you rustup target remove them, it should just skip them. I got the cross tests working in #429, just been waiting on @thedataking's review, so with that they should all work out of the box if you apt install qemu-user qemu-user-static gcc-arm-linux-gnueabihf gcc-aarch64-linux-gnu. Still, there are a couple of errors that that surfaced: #430 (the unused argument error you're getting) and #431 (a runtime test error).

I've also tested things time-to-time on WSL 2, Ubuntu 20.04, but with clang 14 (though I've tested versions 6-14 in the past).

I've seen those same errors in CI locally before when I messed around with the derive macros' imports, so I'm guessing it's there.

[kkysen]

Also, just checked, path! is unclickable in IDEs (vscode, but I assume clion is similar) when using idents instead of path, though when you use path you don't get the individual path segments.

[kkysen]

Ah, the reason the #[derive(BitfieldStruct)] isn't working anymore is because the proc macro is panicking. You must've changed something in that code that caused a panic.

[kkysen]

The error from tests/statics (the above is in tests/structs) is from this test failing:

c2rust/tests/statics/src/test_sections.rs

Lines 32 to 42 in e42598e

	// This static variable is private and unused (but with the used attribute)
	// so there's no way to directly test it was generated except through looking
	// directly at the source file
	let src = include_str!("attributes.rs");
	let lines: Vec<&str> = src.lines().collect();
	let pos = lines
	.iter()
	.position(|&x| x == "static mut rust_used_static4: libc::c_int = 1 as libc::c_int;")
	.expect("Did not find expected static string in source");

[afetisov]

I think we want to make relative paths harder to use, as we should be using absolute paths in almost all of the cases.

I think there should be some limits on that policy. For example, all of u8..usize and i8..isize types are technically just identifiers which may be shadowed in user code, but it would be unreasonable to use them always via absolute paths. It is much more reasonable to forbid shadowing them during the translation. Vec and String are more debatable. Arguably the biggest benefit of using their absolute paths is that they would be automatically usable on systems which don't have std, but do have alloc.

There is also the case of identifiers, identifier expressions and identifier patterns which can also be produced as path![ident].

Imho it is more reasonable to prevent specific crates (e.g. libc and memoffset) to be used via relative paths, but that can be easily achieved just by grepping the codebase for path![libc. The problem is, since the path! macro isn't a unique way to use some item, it is quite easy to circumvent the protections simply using the raw Builder::path and similar methods. In the end, the only guards are human vigilance, and perhaps a grep of transpiled code for relative paths starting with those crate names.

[kkysen]

I think there should be some limits on that policy. For example, all of u8..usize and i8..isize types are technically just identifiers which may be shadowed in user code, but it would be unreasonable to use them always via absolute paths. It is much more reasonable to forbid shadowing them during the translation. Vec and String are more debatable. Arguably the biggest benefit of using their absolute paths is that they would be automatically usable on systems which don't have std, but do have alloc.

I think primitive types like u8 and isize are much less likely to be shadowed, and std::any::type_name doesn't qualify them with ::core::primitive::* as it does with the other types. I agree using them by absolute paths would be too much. The other types aren't used that much, though, so I think using absolute paths there is preferred. As you said, then things will work fine with alloc or core but no std.

There is also the case of identifiers, identifier expressions and identifier patterns which can also be produced as path![ident].

Sorry, I'm not sure what you mean by this?

Imho it is more reasonable to prevent specific crates (e.g. libc and memoffset) to be used via relative paths, but that can be easily achieved just by grepping the codebase for path![libc. The problem is, since the path! macro isn't a unique way to use some item, it is quite easy to circumvent the protections simply using the raw Builder::path and similar methods. In the end, the only guards are human vigilance, and perhaps a grep of transpiled code for relative paths starting with those crate names.

You're right one can always get around things using Builder::path, but the point is to make the easiest, simplest, most-often-used method the most correct, and for the majority of cases, that's absolute paths. I've already ran into the shadowing bug with f128, and it's quite hard to debug at first as the error messages are very much not great, so using absolute paths is worth it.

In fact, there's a good chance some C libraries have typedef unsigned char u8 or typedef uint8_t u8 and similar, and those would cause significant issues with shadowing. Obviously, we don't want to have to shadow the primitive types, so I'm not sure what to do about those. typedef uint8_t u8 can at least be fixed by #259 by translating that to the Rust u8, but typedef unsigned char u8 could be a thorny issue.

[fw-immunant]

There are a number of mostly separable things going on here:

• builder: remove impl Make for &T: this pattern was a holdover from when we used rustc's AST and its P arena pointer type; as we aren't likely to move away from syn, this is a reasonable cleanup. LitStringable (ew) was another holdover from this era, when literal construction was much more complex.
• builder: specialize builder functions where unambiguous: While this has the advantages of removing generics, it changes semantics in a surprisingly subtle way that we shouldn't let slip by unmentioned: if code is changed to pass pre-made syntax nodes into the builder for a given node, they are used exactly as-is rather than being constructed with the same options as the outer builder--this relates to span, mutability, etc.. I reviewed this PR for any places where the generated code might change as a result, and didn't see any, but it's important to note the possibility. Arguably, the previous behavior was too subtle already, because it was already possible (and frequent) to fully build syntax nodes before passing them into other builder methods. This makes things less magic, which is good.
• Building syn::paths with a path! macro (the basic contribution of this PR): I think this is a reasonable improvement--we build paths all over the code and a macro-by-example works well for communicating intent here, while being substantially more concise than building Vecs of strings.
• Adding impl<T: Make<Path>> Make<N> for T for various N: I'm not sure this is worth the concision it enables; inferring which kind of syntax node we produce is not the kind of flexibility that makes code easier to read. It conflates syntax nodes that are paths with syntax nodes that contain paths. I would rather not do this.
• Using reflection (std::any::type_name) or similar to construct paths: I think this is an unwise conflation of the object and meta languages. It makes the transpiler depend on crates just so the transpiled code can use them, and still does string parsing at runtime, without even ensuring that the paths we construct are available in the scope where we emit them. An interface that prevented emitting unusable paths would be more capability-oriented, where adding an import to a scope of generated code gives you a token that can be used inside that scope to access. I don't think such an API would be worth the trouble, though--the main problem this PR addresses is a lack of concision, and we don't often run into bugs with emitting wrong or out-of-scope paths.
• Discouraging relative paths and whether path! should default to absolute: as we currently have both path and abs_path builder methods, I think naming a macro relative_path or rel_path would be confusing. We could have abs_path![::…] and path![…] to be particularly explicit, but I don't think that simply having path![…] and path![::…] (as in the current state of this PR) is a big problem.

[kkysen]

Adding impl<T: Make<Path>> Make<N> for T for various N: I'm not sure this is worth the concision it enables; inferring which kind of syntax node we produce is not the kind of flexibility that makes code easier to read. It conflates syntax nodes that are paths with syntax nodes that contain paths. I would rather not do this.

I agree with this. Inferred generics here makes things over-complicated. For example, there's no way to specify the generic with a turbofish ::<T>() when using a macro, so you lose flexibility, too.

Using reflection (std::any::type_name) or similar to construct paths: I think this is an unwise conflation of the object and meta languages. It makes the transpiler depend on crates just so the transpiled code can use them, and still does string parsing at runtime, without even ensuring that the paths we construct are available in the scope where we emit them. An interface that prevented emitting unusable paths would be more capability-oriented, where adding an import to a scope of generated code gives you a token that can be used inside that scope to access. I don't think such an API would be worth the trouble, though--the main problem this PR addresses is a lack of concision, and we don't often run into bugs with emitting wrong or out-of-scope paths.

True, this is probably not worth it, and with reflection, we could run into problems if cross-compiling/transpiling (as I found out with libc::reallocarray, for example (#497)).

Discouraging relative paths and whether path! should default to absolute: as we currently have both path and abs_path builder methods, I think naming a macro relative_path or rel_path would be confusing. We could have abs_path![::…] and path![…] to be particularly explicit, but I don't think that simply having path![…] and path![::…] (as in the current state of this PR) is a big problem.

I disagree with this. The reason we currently have a hodgepodge of some absolute paths and some relative paths, when almost all of the paths should be absolute, is precisely because of reasons like this. I think the builder methods should also be renamed to prioritize relative paths, so abs_path becomes path and path becomes rel_path. When most of the code uses a certain function, path, without restrictions on absolute vs. relative, this encourages new code to do the same, and introduce the same bugs again. I think we should make it deliberately harder to create relative paths.

[afetisov]

@kkysen almost all of the paths should be absolute

Correct me if I'm wrong, but the only reason to prefer absolute paths is to avoid unintended shadowing of our required crates by some names in the transpiled C code. There is an imho easier and more systematic way to deal with this: add all required crate names to the list of reserved identifiers, and generate fresh renames for all collisions. There is just a handful of reserved crate names, and they aren't that likely to be shadowed in the first place.

Absolute paths are a bit of an odd feature of the language. They are very unidiomatic. I struggle to remember any other use case for them outside of c2rust. In user code you fully control the declared names, so you don't need them. They could be useful in macros, but in practice a macro would use the external crate declared in its consumer, and it's just unreasonable to assume anything about end-user's extern crates. For that reason the required paths will be exported from the macro crate itself, and the macro will use paths relative to $crate.

So absolute path is almost never used, most users will probably be confused when they see it, the reflexes will always lead one to use relative paths, and it isn't even clear which items should or shouldn't use absolute paths (c.f. the u32 example, or Vec). That's a recipe for error. The proper solution is to avoid requiring them in the first place.

inferring which kind of syntax node we produce is not the kind of flexibility that makes code easier to read. It conflates syntax nodes that are paths with syntax nodes that contain paths.

But that is a distinction which doesn't matter in practice. The API is strongly typed, and there is no possibility of using the macro in a place which can accept different node types (and if one was to change the API and introduce such places, the code wouldn't typecheck since the intermediate node type couldn't be resolved).

What matters is that the object is given by the specific path, and it is obvious from the path whether it's an expression, or type, or macro (both because of naming conventions and because the paths that c2rust can hardcode are all well-known objects from stdlib and a few crates). We could make separate path!, path_expr!, path_type! etc macros, but imho that would just be syntactic noise (not that it a significant noise, though).

For example, there's no way to specify the generic with a turbofish ::<T>() when using a macro, so you lose flexibility, too.

That is a more practical issue, indeed. This means that the macro cannot be used in a position of generic arguments. In particular, one cannot call trait methods on path![..] without some sort of type ascription. I'm still thinking about a better way to express that API.

One possibility is to re-introduce generics into the API, but in a more strategic way which would actually allow to reduce boilerplate.

[afetisov]

I have found the cause of errors. It was actually caused by my fix of relative to absolute item paths. The relative paths were hardcoded in test fixtures, and were also incorrectly handled by the bitfield derive macro. Perhaps the test fixtures are too brittle, although one can also say that a change between relative and absolute item paths is something that should be detected by tests.

[fw-immunant]

Please make unrelated changes (8307e28...f2778f6) as one or more PRs separate from this one.

I think the absolute-vs-relative paths question should be addressed separately as well. I like the idea of avoiding accidentally stepping on shadowed identifiers while using idiomatic relative paths where possible, but that doesn't depend on whether we construct paths with a macro or in longhand. For this PR, I would ask that relative/absolute paths are left as-is so that it's easier to review that the macro change is semantics-preserving.

re: implicit conversions from paths to syntax nodes that contain them, I still don't think that this is good for readability. The confusion is not about what kind of (object-language) entity is indicated by the path, but rather the type of the (meta-language) value returned by the path! macro itself.

For example, the change:

-                let fn_path = mk().path_expr(vec!["f128", "f128", "from"]);
+                let fn_path = path![::f128::f128::from];

is confusing as what was once a syn::Expr is now a syn::Path but the adaptation between Expr and Path disappears into the conversion implicit in the Make impl.

Something like:

let fn_path = mk().path_expr(path![::f128::f128::from]);

would leave it clear that fn_path is an expression, to be used in an expression context (rather than, say, a use statement or a pattern).

It's possible to deduce what ends up happening with the additional Make impls (after all, the typechecker does), but using them decreases locality of reasoning, hurting readability.