fix inline directive for a function only called from other packages

[RaduBerinde]

When a top-level function is called from another packages, the
selector expression pkg.FuncName is not handled correctly.

This commit fixes this case by checking the Ident in the selector
expression.

Fixes #17

[RaduBerinde]

A question: is there a reason we don't just go by the cannot inline / can inline output where each function is defined? That would allow the assertion to work even if we don't pass to gcassert any packages that call the function. (currently Pebble only passes the packages that contain a directive, since it is much faster)

[yuzefovich]

Fix LGTM

A question: is there a reason we don't just go by the cannot inline / can inline output where each function is defined? That would allow the assertion to work even if we don't pass to gcassert any packages that call the function. (currently Pebble only passes the packages that contain a directive, since it is much faster)

Do I understand correctly that you're wondering why do we currently apply inline directive to each call site of the target function as opposed to simply looking at the compile output for the target function definition?

I tried to find the answer whether these two things would provide the equivalent coverage but didn't find a definitive answer (golang/go#21536 and https://dave.cheney.net/2020/05/02/mid-stack-inlining-in-go probably provide the most context, apart from reading the runtime code). For some reason I always thought that an eligible to inline function might not be inlined in some call sites, and therefore the change you're wondering about wouldn't be an equivalent one. Perhaps we could have a flag to determine the meaning of inline directive (gcassert ... -inline-global)? Or introduce a separate inlinable or something directive that would assert that the annotated function can be inlined?

Also other directives (bce and noescape) still need compilation of each affected package, so we wouldn't be able to generalize this speedup. Another thing to mention is that in CRDB repo we have a linter that ensures that all affected packages (those that have any gcassert directives) are included into the gcassert ... run.

[RaduBerinde]

TFTR!

It makes sense that it might not be inlined in all callsites. Still, it feels that a cannot inline message should cause a failure even when there are no callsites.

Also other directives (bce and noescape) still need compilation of each affected package, so we wouldn't be able to generalize this speedup. Another thing to mention is that in CRDB repo we have a linter that ensures that all affected packages (those that have any gcassert directives) are included into the gcassert ... run.

These other directives work if we only include packages that contain gcassert directives (this is what Pebble currently does). But inline on a function essentially requires that all callsites are included, and there's no easy way to figure out those packages.

[yuzefovich]

Still, it feels that a cannot inline message should cause a failure even when there are no callsites.

Oh, I think I misunderstood your suggestion initially - I read it as "instead of" but it would work well as "in addition to". I'd definitely be in favor of failing if the function definition that has inline directive gets cannot inline message.

[RaduBerinde]

Oh, I think I misunderstood your suggestion initially - I read it as "instead of" but it would work well as "in addition to". I'd definitely be in favor of failing if the function definition that has inline directive gets cannot inline message.

Yeah that was my thinking initially but I didn't consider the point you made - that the inlining decision could (at least in principle) differ at each callsite.