-
Notifications
You must be signed in to change notification settings - Fork 5k
[API Proposal]: "*App*" wildcard for [UnsafeAccessor] assembly name #106216
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
Comments
Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices |
The #90081 propsal is written in a very narrow way. The input string would simply be passed to The Static version seems okay, but does come with serious costs around manipulating the supplied The Dynamic version is ill-advised because calling the same function would seem to change the content of the method. This would be a redesign of the entire feature. Once an I will need to read over the issues here, but the Dynamic version is a non-starter in my opinion. The Static one seems feasible. |
In my initial thinking, the Dynamic mode would call the resolution callback once per UnsafeAccessor method (when the IL is generated and the target is resolved). It wouldn't call it every time the method is called. |
Why is it not an option to use a fixed assembly name to generate the app specific WinRT interop? |
Okay, so that is the latter case. For me the concern comes down to I can call the function with the same inputs, but its behavior changes if Assembly A calls it vs if Assembly B calls it. That is how I read that feature and I'm not a fan. It creates a very complex scenario. Everytime an |
It wouldn't change based on caller of the UnsafeAccessor method. Assembly A and B would both have their own UnsafeAccessor methods that happen to be described the same but declared in different assemblies. In any case, @jkotas's idea would be significantly more straightforward if it would solve the scenario. |
I don't think I understand this. So let's say assembly A and assembly B both have the following public type: public class Accessor
{
[UnsafeAccessor(UnsafeAccessorKind.StaticMethod, Name = "get_Initialized")]
[UnsafeAccessorType("WinRT.GenericHelpers.IReadOnlyList_System_Collections_IList, *App*")]
public static extern bool IReadOnlyList_System_Collections_IList();
} If I call |
Why won't it? I thought I had proposed this offline a while ago as well. |
Mmh could you elaborate on how would this work? Suppose I have some project B (either an app, or perhaps a WinRT component), where I reference library A.dll, which has that |
Project B would have targets that run after CoreCompile that would generate and compile a sidecar assembly (likely with a temp csproj like how WPF XAML works today) that is only referenced by UnsafeAccessor methods. It would be copied out to the build and publish directories, added to the deps.json, and passed to crossgen2 and ILC as a reference. |
Mmh I see, that's interesting. Just so I understand, any particular reason why this would have to be after CoreCompile and then manually copied and added to deps.json, etc.? Could it not be compiled before CoreCompile and then the assembly added as |
You'd need to do it after CoreCompile if you want to analyze the "app" project easily and have it's generic helpers in this shared sidecar assembly as well. |
I see. Ok this idea is growing on me, and I could see this giving us a way to introduce all kinds of global program view optimizations for CsWinRT in the future. Is there some documented way to do this that we could take a dependency on in CsWinRT? That is, this part slightly worries me:
Are there public docs on what properties we'd need to set, where exactly we'd need to copy that .dll to, etc.? |
This would basically be done with MSBuild targets like how WPF does it. You'd want to hook after There shouldn't be any .NET SDK work necessary here. |
@Sergio0694 Based on the conversation about it doesn't look like this addition to the API is going to be considered. Feel free to re-open if needed, but I'm closing for now. |
Yup, that works for me! We'll be working on an implementation using an extension .dll as suggested 🙂 |
@jkotas @AaronRobinsonMSFT I've discussed possible approaches with Roslyn folks (@sharwell, @jaredpar, @jasonmalinowski, and others), and it seems like there's basically no good way to implement the sidecar .dll approach that would've been the replacement for this proposal. The main issues are basically that you end up choosing between two (bad) options:
In light of this, could we perhaps reconsider this proposal? Was there any technical limitation that made this not feasible? Note that if it helped make things simpler, it'd also be acceptable for us to have some MSBuild property set in the .csproj to specifically tell the runtime "what .dll is the entry .dll", and we could just set that from the .csproj (or from our .targets in CsWinRT and/or the .NET SDK). Thank you! 🙂 |
For now, I would explore option (1) since it is technically feasible. It does have complexity, but that is true for any number of MSBuild apporaches that we employ - this isn't changing the existing complexity in the system by all that much.
By how much? Is this an existing pain point? Are there other wins in this area for CsWinRT 3.0 that may compensate for this impact? |
You may want to take a look at how similar problems are solved in Android and iOS interop. Opinionated interop systems often want to generate some additional code based on the global application view as an optimization. I agree that the msbuild integration is not simple and it has problems, but it works, and it is very flexible. If we were to invent features to enable generation of code based on the global application view, I would like to see Android and iOS to take advantage of them too. BTW: I think it is unlikely that we will get to implementing #114179 in .NET 10. (@AaronRobinsonMSFT please correct me if you think otherwise.) |
I'm also dubious we will get all of those check boxes done. It is going to depend on how/if priorities shift and resourcing. I'm currently investigating the cost for |
It's more something that we're worried about in advance, since Roslyn folks have also mentioned this as something to consider. We'd like to keep builds fast enough, if possible. But yes, there definitely are wins here that would compensate with a (hopefully minimal) regression in build times. The whole new architecture for 3.0 would allow every single CCW vtable and every single
Right, I've been doing that too and talking with both Roslyn folks, and folks from Android/iOS, to figure out the best approach. The idea is that given that all this stuff in CsWinRT 3.0 would be new, I'd really like to make sure that we get on a properly supported path from the start and don't end up getting ourselves stuck in a corner with an improper setup. It seems that:
It seems at least for now our plan might be:
Yeah, that's exactly the kind of thing I had in mind, just like the whole type map was designed to support all the various interop stacks (Android, iOS, WinRT, etc.), I was hoping there could be a system to handle this problem too that could be shared, so that we'd avoid having every single team doing this, needing to reinvent the wheel, and ending up with a standalone system that also means, having to likely stumble upon the same bugs as the other ones (with things such as incremental builds).
Yeah to clarify, we don't expect all those boxes to be completed in .NET 10. As long as the ones that would be blocking for us are implemented (eg. the type map), it would be fine for us. Some of the items there are nice to have but not blocking.
Oh that's awesome to hear!! I assumed there'd be no chance for that in .NET 10 at all 😄 For context: having |
Do you think that this solution is less fragile than invoking Csc directly?
If you just need to generate a bit of simple IL to stitch things together, I would avoid Cecil dependency for that. You can use System.Reflection.Metadata directly. Of if you want something a bit higher level, use MetadataLoadContext and PersistedAssemblyBuilder. |
Maybe? The main argument would be that it would at least be a supported scenario, whereas manually calling
The sidecar .dll would need a whole bunch of code being generated (all marshalling code for generic instantiation). But yeah for sure we could look into those other APIs too. I think @jkoritzinsky suggested Cecil mostly because it's what everyone also uses for this kind of thing, so it might simplify some things if we used the same tooling as other similar projects 🤔 |
Csc task is documented at https://learn.microsoft.com/en-us/visualstudio/msbuild/csc-task . I do not see anything in that documentation that says that it is unsupported. It may be a good idea to drill into the concerns with calling Csc task directly. I believe that these concerns are centered around computing list of dependencies from nothing. It is close to impossible to do it correctly without using the SDK/NuGet integration. Once you are using the integration, you are not going to call Csc directly. I do not think that these concerns apply here. You do have list of dependencies to pass to the compiler, computed using the sanctioned method earlier. You just want to compile one more file using those same dependencies.
Many older projects use Cecil to emit IL since there was no other viable option years ago when they started. It is not the case anymore. |
I think it's probably better for @jaredpar to chime in on that. I do agree with you that if the task is publicly documented (I actually didn't know that), and given that we're simply forwarding all dependencies and properties from MSBuild for the current project, and we're not actually resolving them ourselves, using Csc directly would make sense. I mean that's pretty much exactly what we're trying to do: "please build an additional .dll with these additional source files in the same way the current .dll for this app/library has been built, ie. with all the same exact .NET SDK, build properties, referenced .dll-s, etc.". Basically those same source files should've built correctly had they been in the same app project too, except we need them to be in this sidecar .dll instead, but the rest is the same. If we could actually confirm that in this specific scenario, this use of Csc is actually fine, we'll happily take it. Building the sidecar from source would be infinitely more convenient for us rather than trying to emit IL directly, plus this would put us on the same path as the iOS tooling, meaning we could likely share efforts going forwards to address any common issues or concerns. If we can confirm that this approach is fine and a supported scenario, I'm more than happy to close this issue again and remove it from the epic then. Especially if there's a chance we can make Also, I do appreciate you all taking the time to provide advice and help us figure out the right path forwards 🙂 |
I will work with @BillWagner on documentation updates here. It's likely won't be put into fully unsupported but needs to be in the strongly discouraged category. Having an invocation of the
That is certainly part of the problem. The bigger problem though is that the set of arguments that are effectively required to run the
Essentially, there are many user and ecosystem properties which change how the compiler is supposed to be invoked. Custom invocations of |
@jaredpar The alternative proposed above is:
Do you believe that the auto generated .csproj file is less fragile compared to direct invocation of Csc? I think there is very similar catch-up job in both cases. If there is a new thing introduced, there is like 50:50 chance that this auto-generated .csproj project will want the new thing disabled or configured in some special way. The difference between auto-generated .csproj file and direct Csc invocation is in the ease of diagnosing and fixing eventual issues. Direct Csc invocation is straightforward to see and fix. Auto-generated .csproj is behind a layer of indirection. Diagnosing and fixing the issues means debugging and changing the .csproj file auto-generator. |
@jkotas adding a few more questions here since we were already on the same topic 😄
Additionally, I'm starting to think perhaps we should just bite the bullet and emit IL directly as well. It would allow us to completely bypass all the concerns raised about either using
If we went this route, meaning we'd be generating much more IL, would your recommendation still be to just use (sidenote, I find it kinda funny how we're basically ending up rewriting MCG from scratch at this point ahah) |
Observation: Projects that generate a lot of complicated IL get tired of it eventually and switch to generating C#. If you want to generate IL, it needs to be super simple just to stitch things together to survive the test of time.
The main advantage of Cecil is that it is a mutable object model that enables IL rewriting. If you just want to emit a bunch of IL from scratch, Cecil has less advantage compared to System.Reflection.Metadata, Note Cecil is a community library. We have internal Cecil fork for IL linker to give us full control over servicing for our purpose. |
That is also fair. I suppose it's just not entirely clear to me what the best compromise is between all of these concerns. Emitting IL would avoid the issues around invoking
Ah, I see. That helps, I guess we could just use |
Leaving an update here for reference and to resolve this. It seems the approach we're going to go with is:
Thank you again everyone for all the help and the advice! 🙂 |
@jkotas small follow up question: would you have any recommendation for how to analyze IL? |
Do you mean that you need to keep track of the object type that's get passed into the cast? You need to build IL importer for that. kind of like what's in IL verify or the JIT. I do not think Cecil has one. I do not know about ILSpy. |
Roslyn folks shared a link to their IL visualizer (here), and it seems not too difficult to reuse the logic here to implement an importer that just tracks the evaluation stack. The idea would be to load all import .dll-s with That seems unlikely to make it to .NET 10 I suppose. Perhaps we should explore some other libs to analyze the .dll-s 🤔 |
The code you have linked does not track the evaluation stack. Tracking the state of the evaluation stack is more complicated. |
Yeah no, of course. I didn't mean that the code was doing everything we needed. I was just saying that I was going to port that (since it already has the basic setup to decode IL blobs into actual opcodes and operands), and then build the evaluation stack analyzer on top of it, by traversing the graph of each method body and tracking what actually happens. And then use the |
Background
As part of CsWinRT 2.1.1, we introduced AOT support in WinRT scenarios. In order to make this happen, CsWinRT now also generates a fair amount (read: a metric ton) of code to make things such as WinRT generics work. This support comes with several drawbacks though. The first one is that it is viral: we need every single library to reference CsWinRT in order to make types AOT compatible when used for WinRT interop. This doesn't just affect libraries that are specific to Windows, but any library which contains types that will be used in WinRT scenarios. For instance, I've had to add the Windows TFM to the MVVM Toolkit, so that the types in the MVVM Toolkit could get the necessary supporting code generated. This isn't really fixable, it's just the state of things now that WinRT support is lifted.
On top of this however, we have two main problems that we'd like to solve:
For instance, here's just some of these supporting types generated in the MVVM Toolkit:
It's very easy for a large app to have even hundreds of these spread across all your dependencies and projects. And it's not uncommon that a lot of these will just be copies and copies of the same combinations of type arguments (eg.
string
).We'd like improve things and move as much code generation as possible down to the published projects.
API proposal
The proposal is not an API, but an extension on top of #90081. We'd like to add a new "*App*" wildcard that can be used in place of assembly names for
[UnsafeAccessor]
methods. The exact semantics would be controlled via a new feature switch that can be configured via the newUnsafeAccessorOutputAssemblyResolutionType
property. There would be two modes:Example use
Consider the current CsWinRT code generation for some enumerable type, such as
ObservableGroup<TKey, TElement>
:Generated CCW vtable (click to expand):
This, along with all of those
IReadOnlyList_System_Collections_IList
etc. implementations (each of those is and lot of code as well). With this proposed feature, we'd be able to not generate any of that, and simply do this instead:For any generic type instantiation we need. Then, replace those
Initialized
accesses with:And just call those, which would call the shared generated generic type instantiation in the final assembly. This support could technically us to go even further and potentially just have the entire vtable implementation call to some well known generated method in the final assembly, if we found that method to be even better for CsWinRT (either would work).
Essentially, this allows us to:
Additional information
This design includes suggestions from @jkoritzinsky, who pointed out that in several scenarios around ALC, there is no "entry assembly", or you might have a given dependency library being loaded in one ALC and then referenced by some other code in another ALC. In those cases, the "Dynamic" mode would allow eg. CsWinRT to implement an appropriate resolver callback.
cc. @AaronRobinsonMSFT @jkotas @MichalStrehovsky @agocke
The text was updated successfully, but these errors were encountered: