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

typeck: audit the use of a_is_expected pattern #12934

Closed
edwardw opened this issue Mar 16, 2014 · 1 comment
Closed

typeck: audit the use of a_is_expected pattern #12934

edwardw opened this issue Mar 16, 2014 · 1 comment

Comments

@edwardw
Copy link
Contributor

edwardw commented Mar 16, 2014

In typeck passes, one often needs to do binary operations between two thingy: equality, substitution and what have you. A pattern has been adopted to handle that:

fn some_op(a_is_expected: bool, a: SomeType, b: SomeType) -> SomeResult

Unfortunately there are two interpretations of the signature, one is that the order of a and b is significant, the other insignificant. And the caller and callee seem to diverge.

For example, in librustc/middle/typeck/infer/mod.rs, there's honest one that says "I don't care about the value of a_is_expected":

pub fn mk_subr(cx: &InferCtxt,
               _a_is_expected: bool,
               origin: SubregionOrigin,
               a: ty::Region,
               b: ty::Region) { ... }

There's equality test that the order genuinely doesn't matter:

pub fn mk_eqty(cx: &InferCtxt,
               a_is_expected: bool,
               origin: TypeOrigin,
               a: ty::t,
               b: ty::t)
            -> ures { ... }

And then there's ambiguous one:

pub fn mk_sub_trait_refs(cx: &InferCtxt,
                         a_is_expected: bool,
                         origin: TypeOrigin,
                         a: @ty::TraitRef,
                         b: @ty::TraitRef) -> ures
{
    ...
    let suber = cx.sub(a_is_expected, trace);
    suber.trait_refs(a, b)
    ...
}

It seems to use parameter a_is_expected, but only for error messages. The real deal trait_refs actually dictates a fixed order of a and b, which makes calling it like mk_sub_trait_refs(..., false, a, b) semantically wrong (it should be mk_sub_trait_refs(..., true, b, a)). Or rather, what caller expects is:

{
    if a_is_expected { suber.trait_refs(a, b) }
    else { suber.trait_refs(b, a) }
}

I believe that's what leads to bugs like #12223.

So we should check and fix it, either by removing the a_is_expected all together or by rectifying the order of a and b when it is used wrong.

@steveklabnik
Copy link
Member

I believe these are now gone:

pub fn mk_subr<'a, 'tcx>(cx: &InferCtxt<'a, 'tcx>,
                         origin: SubregionOrigin<'tcx>,
                         a: ty::Region,
                         b: ty::Region) {

lnicola pushed a commit to lnicola/rust that referenced this issue Aug 9, 2022
Add a setting to disable comment continuation in VSCode

Fixes rust-lang/rust-analyzer#12928
flip1995 pushed a commit to flip1995/rust that referenced this issue Jul 11, 2024
…tializer_can_be_made_const, r=Alexendoo

Rename thread_local_initializer_can_be_made_const to missing_const_for_thread_local

Close rust-lang#12934
As discussed at rust-lang#12934 name `thread_local_initializer_can_be_made_const` sounds against convention for other lints which describe the issue/wrong code but not suggestion and it is quite long. The new name take example from existing lint `missing_const_for_fn`

changelog: `thread_local_initializer_can_be_made_const` : Rename to [`missing_const_for_thread_local`]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants