Initial draft for a capability-based analyzer #261
Conversation
| { | ||
| } | ||
|
|
||
| [RequiresDynamicCode] |
There was a problem hiding this comment.
We typically only mark the constructor or factory to limit number of places that need to be annotated.
|
|
||
| She immediately gets a diagnostic: | ||
|
|
||
| CAXXXX: The API 'Thread.Start()' requires the capability 'ThreadCreation'. Either mark the consumer with `[RequiresThreadCreation]` or check by calling 'Thread.IsCreationSupported'. |
There was a problem hiding this comment.
Are there other platform APIs except Thread.Start that would be marked as requiring the thread create capability?
There was a problem hiding this comment.
I think Task.WaitAll could be in the bucket
There was a problem hiding this comment.
I see that Task.WaitAll is marked as supported on browser, but Task.Wait is not. Why is that?
I would expect Task.WaitAll to be just a more efficient version of foreach (Task t in tasks) t.Wait().
Are both Task.Wait and Task.WaitAll going to be marked with this capability?
There was a problem hiding this comment.
I believe the idea behind is that if you are in a single-threaded setup Task.WaitAll will never return as you wait on more than 1 concurrent thing (yeah there are some exceptions). Therefore if you are calling it you are doing something wrong.
/cc @lewing
There was a problem hiding this comment.
I believe the idea behind is that if you are in a single-threaded setup Task.WaitAll will never return as you wait on more than 1 concurrent thing (yeah there are some exceptions).
That should not be the case. As Jan suggested, behaviorally there shouldn't be a difference between using Task.WaitAll on multiple things and using Task.Wait on each of those multiple things.
Further, both WaitAll and Wait can "inline" the thing they're waiting on if it's backed by a delegate to be executed (e.g. Task.Run(action)), so if you are in a single-threaded environment and you're waiting on such tasks, they should both "just work". If they don't in such cases, that suggests we're missing handling of this case in browser.
There was a problem hiding this comment.
I'm surprised WaitAll isn't marked. There are definitely certain circumstances where it can block. I suspect it got overlooked.
Further, both WaitAll and Wait can "inline" the thing they're waiting on if it's backed by a delegate to be executed (e.g. Task.Run(action)), so if you are in a single-threaded environment and you're waiting on such tasks, they should both "just work". If they don't in such cases, that suggests we're missing handling of this case in browser.
In the browser we have tasks (e.g. any JS interop involving a Promise) that cannot complete without yielding execution back to the browser main loop, so they won't "just work". Wait and WaitAll can work under many circumstances but in practice they can and do break in confusing ways.
There was a problem hiding this comment.
so they won't "just work"
Right, that's the category I mentioned where they're not backed by delegates.
Both WaitAll and Wait would have the same issue there; WaitAll isn't special in that regard.
| // - ILGenerator | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
We also have RequiresAssemblyFilesAttribute and RequiresUnreferencedCodeAttribute and associated analyzers. Are these attributes in scope or out of scope for this proposal?
There was a problem hiding this comment.
Depends on whether what they track can be mapped to the capability analyzer. The goal wasn't to replace the existing linker analyzers as much as it was to replace the [UnsupportedOSPlatform("browser")] annotations with something more scoped/future proof. I merely added the dynamic code capability because it was a recent example of a capability-based API that this design should be able to accommodate as well.
|
|
||
| [AttributeUsage(AttributeTargets.Method | | ||
| AttributeTargets.Property | | ||
| AttributeTargets.Field, |
There was a problem hiding this comment.
Do you have an example of which capability would be controlled by field?
There was a problem hiding this comment.
We have done this for [UnsupportedOSPlatformGuard], for example here. The intent is to cache the result of the call.
|
FYI, we're doing a lot of this with I haven't read through the proposal yet, but I hope the semantics would be compatible with what we've come up with for the above attributes. |
|
Is there any connection to build properties? |
|
|
||
| [RequiresThreadCreation] | ||
| public void Start(); | ||
| } |
There was a problem hiding this comment.
This example makes sense to me, @terrajobst could you add example of CapabilityGuardAttribute usage? Not sure if that was supposed to be added to IsCreationSupported property or we should know that it is the guard by the 2nd constructor argument of CapabilityCheck attribute
No description provided.