Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] change the late resource syntax #202

Closed
japaric opened this issue Jun 14, 2019 · 16 comments

Comments

Projects
None yet
4 participants
@japaric
Copy link
Owner

commented Jun 14, 2019

Current behavior

The current syntax for late resources is very similar to the syntax for normal
resources: one simply uses the unit value (() ) as the "initial value" for the
late resource.

#[rtfm::app(device = ..)]
const APP: () = {
    static mut EARLY: u32 = 0;
    static mut LATE: u32 = ();
};

Rationale

Some people have expressed their dissatisfaction with the current syntax because
it wouldn't type check in normal Rust code; they would expect the initial
value to be omitted as semantically there's no initial value. However, dropping
the initial value would render the RTFM syntax un-rustfmt-able.

#[rtfm::app(device = ..)]
const APP: () = {
    static mut EARLY: u32 = 0;
    static mut LATE: u32; // <- not valid Rust syntax so it can't be rustfmt-ed
};

Proposal

This RFC proposes using the extern static syntax to specify late resources.
This lets us omit the initial value in the syntax while keeping the syntax
rustfmt-able.

Detailed design

#[rtfm::app(device = ..)]
const APP: () = {
    static mut EARLY: u32 = 0;

    extern "C" {
        // runtime initialized resource
        static mut LATE: u32;

        // runtime initialized `static` variable
        static ALSO_LATE: u32;
    }
};

Drawbacks

There would be an ABI attached to the late resources. This gives the false
impression that late resources use the "C" ABI:

struct SomeStruct {
    // ..
}

#[rtfm::app(device = ..)]
const APP: () = {
    // Rust ABI
    static mut EARLY: SomeStruct = SomeStruct::new();

    extern "C" {
        // C ABI?
        static mut LATE: SomeStruct;
    }

    // ..
};

Which is not true because the ABI is a property of the type not of the
declaration.

Changing the declaration to extern "Rust" doesn't really help because the
resource could actually be using a C representation:

#[repr(C)]
struct SomeStruct {
    // ..
}

#[rtfm::app(device = ..)]
const APP: () = {
    extern "Rust" {
        // Rust ABI?
        static mut LATE: SomeStruct;
    }

    // ..
};

Alternatives

RFC #143 proposes using an struct syntax to specify resources. That proposal
has two issues which this proposal doesn't have:

  • The syntax doesn't let the user distinguish resources (today's static mut
    variables) from static variables -- the distinction is important because
    static variables are treated differently in the Send / Sync analysis

  • It gets rid of non-late resources so one can no longer specify resources, or
    static variables, whose initial values are known at compile time -- this could
    have a small negative impact on the initialization time and program size of all
    RTFM applications.


Another alternative would be to special case core::mem::MaybeUninit to mean
late resources so one would write:

use core::mem::MaybeUninit;

#[rtfm::app(device = ..)]
const APP: () = {
    static mut LATE: MaybeUninit<SomeStruct> = MaybeUninit::uninit();

    #[init]
    fn init(_: init::Context) -> init::LateResources {
        init::LateResources { LATE: SomeStruct::new() }
    }
};

However, this gets tricky quickly because the macro has no way to know whether
the identifier MaybeUninit refers to core::mem::MaybeUninit or some user
defined type.

// user defined
struct MaybeUninit<T> {
    always_initialized: T,
}

#[rtfm::app(device = ..)]
const APP: () = {
    static mut LATE: MaybeUninit<SomeStruct> = MaybeUninit::uninit();

    #[init]
    fn init(_: init::Context) -> init::LateResources {
        init::LateResources { LATE: SomeStruct::new() }
    }
};

cc @TeXitoi

@japaric japaric added the RFC label Jun 14, 2019

@japaric japaric added this to the v0.5.0 milestone Jun 14, 2019

@jonas-schievink

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

However, dropping the initial value would render the RTFM syntax un-rustfmt-able.

#[rtfm::app(device = ..)]
const APP: () = {
    static mut EARLY: u32 = 0;
    static mut LATE: u32; // <- not valid Rust syntax so it can't be rustfmt-ed
};

AFAIK there's a deeper issue here: This doesn't parse at all, so it would be impossible to implement because macros, including proc. macro attributes, are only expanded after the code is parsed.


Regarding the RFC itself: I do agree that it's a good idea to change the late resource syntax. The magic () really isn't the best. However, replacing that with extern "ABI-that-isn't-used" magic also doesn't seem like a very satisfying solution to me.

Note that extern {} is also a valid item. Just like extern fn, it defaults to the "C" ABI. The #[rtfm::app] macro could require that no ABI is specified.

@TeXitoi

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

I agree with @jonas-schievink : I don't like the actual syntax, but I don't like this proposition more.

Maybe

#[rtfm::app(device = ..)]
const APP: () = {
    static mut LATE: SomeStruct = rtfm::LATE_RESOURCE;

    #[init]
    fn init(_: init::Context) -> init::LateResources {
        init::LateResources { LATE: SomeStruct::new() }
    }
};

Not a big fan neither.

@japaric

This comment has been minimized.

Copy link
Owner Author

commented Jun 19, 2019

@jonas-schievink

This doesn't parse at all, so it would be impossible to implement because macros, including proc. macro attributes, are only expanded after the code is parsed.

You are correct. I was thinking of bang macros; those do accept any custom syntax.

Note that extern {} is also a valid item.

True, but rustfmt formats these into extern "C" {} meaning that running cargo fmt on your crate would make #[app] fail to expand. We could recommend a rustfmt.toml to not auto-complete the ABI of extern blocks ... but that might result in the user hitting the rustc bug where all span information is lost (rust-embedded/cortex-m-rt#163, rust-lang/rust#43081)

@japaric

This comment has been minimized.

Copy link
Owner Author

commented Jun 20, 2019

Would it be less terrible to use .. instead of () as the initial value for the static variables that actually are late resources?

For completeness sake here are all the variations of today's syntax that we could use (regardless of how (un)reasonable they are)

const APP: () = {
    // expressions that are unlikely to be used in applications
    static mut A: Type = (); // today's syntax
    static mut B: Type = ..; // `core::ops::FullRange`
    static mut C: Type = {}; // also evaluates to the unit type `()`
    static mut D: Type = []; // `[T; 0]`
    static mut E: Type = '_'; // `char`
    static mut F: Type = ""; // `&'static str`

    // super weird but valid syntax
    static mut G: Type = break;
    static mut H: Type = continue;
    static mut I: Type = return;

    // invalid syntax
    // static mut J: Type = _;
    // static mut K: Type = ?;
};

Alternatively, we could use a variation of #143 that uses two attributes: one for late resources (static mut) and another for runtime initialized static variables.

const APP: () = {
    static mut EARLY1: u32 = 0;
    static EARLY2: u32 = 0;

    #[strawman]
    struct StaticMut {
        // === static mut late1: u32 = ();
        late1: u32,
        late2: u32,
    }

    #[proposal]
    struct Static {
        // === static late3: u32 = ();
        late3: u32,
        late4: u32,
    }

    #[init]
    fn init(_: init::Context) -> init::LateResources {
        init::LateResources {
            late1: 0,
            late2: 1,
            late3: 2,
            late4: 3,
        }
    }
};
@jonas-schievink

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

True, but rustfmt formats these into extern "C" {}

Argh, how annoying :(

Alternatively, we could use a variation of #143 that uses two attributes: one for late resources (static mut) and another for runtime initialized static variables.

I think I'm more of a fan of what you said in #143 (comment), where late resources would be put into their own struct. This also neatly solves the somewhat weird requirement that init has to assign to all late resources in its last N statements. Instead, it would just return all late resources packed in a struct, and RTFM would do the assignment. This would just reuse the standard Rust semantics of return values instead of introducing a custom check.

Combining that with the crate-level #![app] attribute would also get rid of the last wart I personally see in the RTFM syntax, which is the const APP: () declaration (actually quite similar to the () late resource initializer we're discussing here).

@TeXitoi

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

.. is funny, I think I prefer it a bit to ().

You can also use attributes to differenciate mut and non-mut:

#[late]
struct LateResources {
    #[mut]
    late1: u32,
    #[mut]
    late2: u32,
    late3: u32,
    late4: u32,
}
@TeXitoi

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

Having late resources in a struct and non-late resources as statics will cause strange naming conventions: late resources will be snake case while non-late resources would be screaming case.

@japaric

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

@TeXitoi

.. is funny, I think I prefer it a bit to ().

.. is easier to miss while skimming the code; that might be why.

You can also use attributes to differenciate mut and non-mut:

And an argument to specify the initial value?

Oh, and we can't use mut because it's a keyword but we can use #mut or some other identifier.

// all resources
#[resources] // <- not really needed, I think
struct Resources {
    #[r#mut(init = SomeType::const_new())]
    early_resource: SomeType,

    #[r#mut]
    late_resource: OtherType,

    #[not(r#mut)] // <- or something else that makes more sense
    late_static: AndAnotherType,

    #[not(r#mut(init = YetAnotherType::const_new()))]
    early_static: YetAnotherType,
}

#[init]
fn init(c: init::Context) -> init::LateResources {
    init::LateResources {
        late_resource: OtherType::new(),
        late_static: AndAnotherType::new(),
    }
}

That parses.

late resources will be snake case while non-late resources would be screaming case.

Even with the above syntax task locals would still have ALL_CAPS names.

#[task(resources = [resource])]
fn a(c: a::Context) {
    static mut LOCAL: u32 = 0;

    *c.resources.resource += 1;
    *LOCAL += 1;
}

I think I can live with the resource syntax not reflecting that resources are all static variables. The current syntax does have a small refactor advantage: one can easily turn a task local into a resource or a plain static variable into a resource by just cut-pasting that line / item.

@TeXitoi

This comment has been minimized.

Copy link
Collaborator

commented Jun 24, 2019

I like this proposition. As an explanation of the not SHOUTING_CASE resources, you can see resources as a struct that is itself a static, and thus the fields are not themselves statics directly, explaining the snake casing.

I'd separate mutable and init, it would do

// all resources
struct Resources {
    #[mutable, init = SomeType::const_new()]
    early_resource: SomeType,

    #[mutable]
    late_resource: OtherType,

    // nothing: static late resource
    late_static: AndAnotherType,

    // static inited, but is it really useful compared to plain static?
    #[init = YetAnotherType::const_new()]
    early_static: YetAnotherType,
}

#[init]
fn init(c: init::Context) -> init::LateResources {
    init::LateResources {
        late_resource: OtherType::new(),
        late_static: AndAnotherType::new(),
    }
}
@japaric

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

@TeXitoi I like the syntax you have proposed but rustc / rustfmt doesn't like these two attributes:

#[mutable, init = SomeType::const_new()]
#[init = YetAnotherType::const_new()]

It seems that if you want arguments (key = value) then the list must be preceded by a path and wrapped in parentheses:

#[path(key = value)]

// static inited, but is it really useful compared to plain static?

(the advantage is access control: you can enforce (declaratively) that the static is only accessible from certain tasks -- you won't get the same if you stick a normal static in the root of the crate)

@korken89

This comment has been minimized.

Copy link
Collaborator

commented Jun 25, 2019

I am not much for the first syntax proposed, as what has extern "C" to do with resources?
IMO it adds unnecessary confusion.

What @TeXitoi is quite good though! Problematic though if it's not liked. Is there a way to get it close to that which rustc/rustfmt would accept?

@japaric

This comment has been minimized.

Copy link
Owner Author

commented Jun 29, 2019

If we have more breaking-changes budget to spare we could mix this with just the shared access syntax of RFC #129.

The main idea would be that there would no longer be a difference between static and static mut resources; there would only be resources (in fact, this would match the underlying implementation: all resources are static mut variables, even the ones declared today as static variables).

This means that we wouldn't need a #[mutable] attribute in the struct Resources declaration. Instead to figure out the Sync requirement we would look at the resources list: &foo is today's static resource and needs a Sync bound in some cases; whereas &mut bar or bar is today's static mut resource and is subject to ceiling analysis.

In a nutshell we would go from this:

#[rtfm::app]
const APP: () = {
   static mut A: Late = ();
   static mut B: Early = Early::new();
   static C: Late = ();
   static D: Early = Early::new();

   #[task(resources = [A, B, C, D])]
    fn foo(cx: foo::Context) {
        cx.resources.a.lock(|_| {});
        let b: &mut Early = cx.resources.b;

        let c: &Late = cx.resources.c;
        let d: &Early = cx.resources.d;
    }
}

to this

#[rtfm::app]
const APP: () = {
    struct Resources {
        a: Late,
        #[init(Early::new())]
        b: Early,
        c: Late,
        #[init(Early::new())]
        d: Early,
    }

    #[task(resources = [a, b, &c, &d])]
    fn foo(cx: foo::Context) {
        cx.resources.a.lock(|_| {});
        let b: &mut Early = cx.resources.b;

        let c: &Late = cx.resources.c;
        let d: &Early = cx.resources.d;
    }
};

We could accept both [a] and [&mut a], which mean today's static mut resource, or just one of them.

And to not add new locking features we would reject mixing [a] and [&a]:

#[rtfm::app]
const APP: () = {
    struct Resources {
       x: X,
    }

    #[task(resources = [x])]
    fn foo(cx: foo::Context) { .. }

    #[task(resources = [&x])] //~ error: [insert message that makes sense here]
    fn bar(cx: bar::Context) { .. }
}

One more thing worth pondering about is how this struct Resources syntax would evolve to accommodate a crate level #![app] attribute. The evolution of today's static syntax would allow putting resources in any module:

#![rtfm::app]

#[resource]
static mut X: u64 = 0;

mod a {
    #[resource]
    static mut Y: u64 = 0;
}

mod b {
    #[task(resources = [X, a::Y])]
    fn foo(cx: foo::Context) {
        // ..
    }
}

Plus the path to static variable syntax looks natural.

It's not clear to me how the struct Resources would scale to multiple modules. Multiple structs? What would be the syntax for list of resources?


Finally, we could leave the resources syntax unchanged for v0.5.0 and revisit it for the v0.6.0 release.

@korken89

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

SGTM, right now I have a hard time to get a feel for how these would be to use. I'll ponder it a bit and come back with more feedback.

@TeXitoi

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

I like the proposition in #202 (comment)

For the problem of #[mutable, init(foo)], that's because, in general, the annotations are nested in a "namespace" (think of #[serde(a, b)]. We can do 2 annotations, i.e.

// all resources
struct Resources {
    #[mutable]
    #[init(SomeType::const_new())]
    early_resource: SomeType,

    #[mutable]
    late_resource: OtherType,

    // nothing: static late resource
    late_static: AndAnotherType,

    // static inited, but is it really useful compared to plain static?
    #[init(YetAnotherType::const_new())]
    early_static: YetAnotherType,
}

#[init]
fn init(c: init::Context) -> init::LateResources {
    init::LateResources {
        late_resource: OtherType::new(),
        late_static: AndAnotherType::new(),
    }
}
@korken89

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

I agree on this approach, using the resource access list does indeed look like a good solution!

@japaric

This comment has been minimized.

Copy link
Owner Author

commented Jul 4, 2019

This extern syntax is not going to fly. Let's discuss the struct + &x syntax in RFC #212

@japaric japaric closed this Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.