This is really annoying bug of hazard build. If you write a lambda as:

nsresult rv = [&]() MOZ_CAN_RUN_SCRIPT_BOUNDARY -> nsresult {
  ...
}();

It cases the following error:

In lambda function:
error: expected '{' before '->' token
nsresult rv = ([&]() MOZ_CAN_RUN_SCRIPT_BOUNDARY -> nsresult {
                                                    ^~

Here is an example.

This is a serious issue when it needs to return mozilla::Result for example. E.g., if I'd write this code:

Result<RefPtr<Element>, nsresult> result = [&]() MOZ_CAN_RUN_SCRIPT {
  if (NS_FAILED(DoSomething())) {
    return Err(NS_ERROR_FAILURE);
  }
  RefPtr<Element> newElement = CreateNewElement();
  return newElement;
}();

This is not allowed because all return statements require implicit conversion to Result unless specifying the return type explicitly.

Therefore, it needs to be:

Result<RefPtr<Element>, nsresult> result = [&]() MOZ_CAN_RUN_SCRIPT {
  if (NS_FAILED(DoSomething())) {
    return Result<RefPtr<Element>, nsresult>(NS_ERROR_FAILURE);
  }
  RefPtr<Element> newElement = CreateNewElement();
  return Result<RefPtr<Element>, nsresult>(newElement);
}();

This is really harder to read and looks messy.

At the moment, the hazard build doesn't even record the attributes on lambdas, so this really isn't doing any good anyway.

I would like to make the analysis start paying attention to both MOZ_CAN_RUN_SCRIPT and MOZ_CAN_RUN_SCRIPT_BOUNDARY, so I don't want to just ignore it. But there's an easy fix -- the hazard analysis supports [[attr_name]] syntax, which is actually specified unlike the currently-used __attribute__((attr_name)) stuff. Our clang static analysis setup unfortunately does not support it (and it's not straightforward to do). But I'll just conditionally switch to that here. Patch coming up...

It looks like this doesn't break anything, at least: https://treeherder.mozilla.org/jobs?repo=try&author=sfink%40mozilla.com&selectedTaskRun=e4gv2SGSR3SDKRPQXvI1uw.0

I think we mark these P1 if they're being worked on? (I have a patch waiting for review that fixes this.)

Bleh. I worked on this today, including the big hammer fix of switching all of our __attribute__ declarations to various [[...]] versions.

But MOZ_CAN_RUN_SCRIPT, in particular, cannot be changed (yet). There is no legal way to annotate a lambda function with the [[...]] syntax. It appears likely that c++23 will add this capability. But right now, there's just no way to do it, which seems rather ridiculous. So much for switching to a properly specced way of doing annotations.

The existing __attribute__ syntax is allowed in that position by clang. For the purposes of this bug, I think I'll have to switch to #defineing the annotation into nothingness for now.

It's still a little weird. If I do bool foo = []() [[nodiscard]] { return true; }(); then clang gives me the warning error: 'nodiscard' attribute cannot be applied to types. Which means that the [[nodiscard]] in that position is being applied to type instead of the function. (They are different.) Which makes me wonder why

bool foo = []() __attribute__((annotate("moz_can_run_script_boundary"))) { return true; }();

works. Unless maybe the clang plugin that processes that annotation is pulling it out of the type?? So confusing.

It is possible to make everything happy, it seems. You would "just" need to do:

nsresult rv = [&]() MOZ_CAN_RUN_SCRIPT_BOUNDARY_FOR_CLANG -> nsresult MOZ_CAN_RUN_SCRIPT_BOUNDARY_FOR_GCC {
  ...
}();

and #define one or the other to __attribute__((annotate("moz_can_run_script_boundary"))). ;-) But I don't want to go there, at least until I attempt to add my own moz_can_run_script analysis.

Per https://searchfox.org/mozilla-central/rev/82946eb5e7d1234f3218310e7bc8a394666dbda5/build/clang-plugin/CanRunScriptChecker.cpp#251 it seems currently it attaches to () part, and somehow the standard [[]] syntax decided to deviate? 🤔

The existing __attribute__ syntax is allowed in that position by clang. For the purposes of this bug, I think I'll have to switch to #defineing the annotation into nothingness for now.

I think that it's fine because the other normal builds can detect a bug of unexpected calls of "can-run-script" method. (And I wonder there is no check about "can-run-script" check at calling lambdas...)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

The existing __attribute__ syntax is allowed in that position by clang. For the purposes of this bug, I think I'll have to switch to #defineing the annotation into nothingness for now.

I think that it's fine because the other normal builds can detect a bug of unexpected calls of "can-run-script" method.

RIght, it doesn't introduce any problems. This would be for bug 1736082. It looks like I need to add more detail there, but in short: the can-run-script annotations are currently checked within a limited domain, delimited by manually annotated boundaries. All functions need to be manually annotated. With a global analysis like the hazard analysis infrastructure provides, we could either (1) drop all existing annotations and have them automatically figured out from the callgraph (this would require a new set of annotations for things that can't be determined statically); or more likely (2) the manual boundaries could be removed and MOZ_CAN_RUN_SCRIPT annotations automatically added or suggested to everything currently behind the boundary. Or a combination.

(And I wonder there is no check about "can-run-script" check at calling lambdas...)

There is.

https://hg.mozilla.org/mozilla-central/rev/32a83960adc5