feat: improve the lifetime of as_ast_kind methods

[shulaoda]

I tried to introduce new methods in PR #5491 , but encountered:

pub fn get_function_like_declaration<'b>(
    node: &AstNode<'b>,
    ctx: &LintContext<'b>,
) -> Option<&'b BindingIdentifier<'b>> {
    let parent = outermost_paren_parent(node, ctx)?;

    if let AstKind::VariableDeclarator(decl) = parent.kind() {
        let ident = decl.id.get_binding_identifier()?;
        Some(ident)
    } else {
        None
    }
}

to

pub fn get_function_like_declaration<'b>(
    node: &AstNode<'b>,
    ctx: &LintContext<'b>,
) -> Option<&'b BindingIdentifier<'b>> {
    let parent = outermost_paren_parent(node, ctx)?;
    let decl = parent.kind().as_variable_declarator()?;

    let ident = decl.id.get_binding_identifier()?;
    Some(ident)
}

error:

temporary value created hererustc[E0515](https://doc.rust-lang.org/error-index.html#E0515)
ast_util.rs(436, 5): original diagnostic
// size = 8, align = 0x8
let parent: &AstNode<'_>

When I changed the method to the following, there was no longer this problem.

pub fn as_variable_declarator(self) -> Option<&'a VariableDeclarator<'a>> {
    if let Self::VariableDeclarator(v) = self {
        Some(v)
    } else {
        None
    }
}

@IWANABETHATGUY cc

[rzvxa]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

[IWANABETHATGUY]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

We don't need to consume itself, just adding the lifetime is enough.

[IWANABETHATGUY]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

I am ok for both, but as_xxx return reference is default behavior in rust-analyzer

[rzvxa]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

We don't need to consume itself, just adding the lifetime is enough.

Oh, I just saw this comment, Please ignore my comment on the merged PR.

I was thinking that since both the &AstKind and AstKind have the same size maybe we can save a few CPU cycles by avoiding the extra dereferencing.

[rzvxa]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

I am ok for both, but as_xxx return reference is default behavior in rust-analyzer

Yes Sir, Since the naming is an anti-pattern this might not be worth doing. Please go ahead and implement it as you see fit.

[IWANABETHATGUY]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

We don't need to consume itself, just adding the lifetime is enough.

Oh, I just saw this comment, Please ignore my comment on the merged PR.

I was thinking that since both the &AstKind and AstKind have the same size maybe we can save a few CPU cycles by avoiding the extra dereferencing.

ok, that make sence.

[IWANABETHATGUY]

You are right, AstKind is cheap to copy - and implements Copy - so this implementation makes sense and is more versatile as it doesn't require the user to clone/copy at the call site.

I am ok for both, but as_xxx return reference is default behavior in rust-analyzer

Yes Sir, Since the naming is an anti-pattern this might not be worth doing. Please go ahead and implement it as you see fit.

performance goes first,

[rzvxa]

@IWANABETHATGUY No I'm sorry, I was hullucinating. The AstKind type is 16 bytes since it needs 8 bytes for the tag. The implementation with the reference is more performant and better overall.

Please ignore everything I said above.

[overlookmotel]

The AstKind type is 16 bytes since it needs 8 bytes for the tag. The implementation with the reference is more performant and better overall.

Out of interest, is your "is more performant" conclusion based on measurement?

I would tend to always make methods on Copy types receive self not &self (unless they're large types, but we probably shouldn't make large types Copy). And I'd assume that's more performant, if both "words" of the type are read in the method, and so need to be read into registers one way or another.

But maybe I'm wrong. If I am, I would very much like to know!

Most of AstKind's method take self.

[rzvxa]

Out of interest, is your "is more performant" conclusion based on measurement?

No Sir, But since it is bigger than a pointer it isn't an open-and-shut case anymore and indirection may or may not be faster. I could've used better wording there.

[overlookmotel]

Thanks for coming back. Hmm I wonder which is more performant. Probably (hopefully) these tiny functions will all get inlined anyway, so it'll make no difference in practice. But it'd be useful to know for other cases.