BAML→XAML decompile: fix nested Type names & MarkupExtension handling #188
Conversation
Nested Type names were quite broken. Of course they are doomed in XAML anyway, but this solves a fair about of the corruption and crashing-out-of-decompile problems. Examples now much improved: System.Activities.Presentation.View.TypeBrowser System.Activities.Core.Presentation.FlowchartDesigner
fix nested Type names in BAML -> XAML decompile
* fix nested Type names in BAML -> XAML decompile
Nested Type names were quite broken. Of course they are doomed in XAML anyway, but this solves a fair about of the corruption and crashing-out-of-decompile problems.
Examples now much improved:
System.Activities.Presentation.View.TypeBrowser
System.Activities.Core.Presentation.FlowchartDesigner
System.Activities.Presentation.View.VBIdentifierDesigner
* Update XamlUtils.cs
use '+' when decompiling from BAML to XAML when some MarkupExtensions contain nested Type names e.g.
--> "{x:Static sad:WorkflowDesignerIcons+DesignerItems.WarningValidation}"
BEFORE... (ToolboxDefaultTemplate.baml in System.Activities.Presentation.g.resources...)
```
<ResourceDictionary x:Uid="ResourceDictionary_1"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:swdv="clr-namespace:System.Activities.Presentation.View"
xmlns:swd="clr-namespace:System.Activities.Presentation"
xmlns:swdt="clr-namespace:System.Activities.Presentation.Toolbox">
<ControlTemplate
x:Key="ToolboxDefaultTemplate"
x:Uid="ControlTemplate_1"
TargetType="{x:Type swdt:ToolboxControl}"
Resources="{swd:CachedResourceDictionary Uid=swd:CachedResourceDictionaryExtension_1, Source=pack://application:,,,/System.Activities.Presentation;component/Themes/Generic.xaml}">
<Grid> <!-- ... --> </Grid>
</ControlTemplate>
</ResourceDictionary>
```
AFTER...
```
<ResourceDictionary x:Uid="ResourceDictionary_1"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:swdv="clr-namespace:System.Activities.Presentation.View"
xmlns:swd="clr-namespace:System.Activities.Presentation"
xmlns:swdt="clr-namespace:System.Activities.Presentation.Toolbox">
<ControlTemplate
x:Key="ToolboxDefaultTemplate"
x:Uid="ControlTemplate_1"
TargetType="{x:Type swdt:ToolboxControl}">
<FrameworkTemplate.Resources>
<swd:CachedResourceDictionaryExtension x:Uid="swd:CachedResourceDictionaryExtension_1" Source="pack://application:,,,/System.Activities.Presentation;component/Themes/Generic.xaml" />
</FrameworkTemplate.Resources>
<Grid> <!-- ... --> </Grid>
</ControlTemplate>
</ResourceDictionary>
```
| @@ -53,6 +54,8 @@ sealed class MarkupExtensionRewritePass : IRewritePass { | |||
| return doWork; | |||
| } | |||
|
|
|||
| static bool NonInlineAttibute(XAttribute attr) => attr.Name.Namespace == XamlContext.KnownNamespace_Xaml; | |||
There was a problem hiding this comment.
How about renaming the function to IsXAMLAttribute to better describe its behavior?
There was a problem hiding this comment.
Settling on IsXamlNsAttr, because names of reflection member entities that aren't classes derived from System.Attribute now appear to be confusing to some of the latest static analysis tools. VS intellisense seems over-eager to strip off -Attribute as an aliasing suffx.
| static string NestedReflectionName(ITypeDefOrRef type, out string clrNs) { | ||
| var name = type.ReflectionFullName; | ||
| while (type.DeclaringType is ITypeDefOrRef t2) | ||
| type = t2; | ||
| clrNs = type.ReflectionNamespace; | ||
| name = name.Substring(clrNs.Length + 1); | ||
| return name; | ||
| } |
There was a problem hiding this comment.
Please rename t2 to declaringType to better reflect what it is :p
| sb.Append(name.LocalName); | ||
| sb.Append(name.LocalName.Replace("_x002B_", "+")); |
There was a problem hiding this comment.
Are we sure that undoing the escaping of the + character is always the right thing to do? The function you modified is used for values of XML attributes (where the + character is allowed since the value is stored within double quotes) as well as in the values of XML elements where I am not sure if the + character is valid since the value is stored raw without being placed in double quotes.
There was a problem hiding this comment.
Yeah I stated that this was sketchy in my intro. I’ll remove this line from the PR.
Don't use unquoted '+' char within the XAML name for nested types--even though that's what the MS toolset seems to expect--because it's not a valid character to be exposed as an identifier in naked XML. A plus sign currently is still deployed into the name, as fetched from reflection via `nSpy.BamlDecompiler.XamlContextNestedReflectionName(...)`. But as a result of multiple XML identifier sterilizations, such cases will now appear in the final XAML as, e.g.: ``` ns:WorkflowDesignerIcons_x002B_DesignerItems.WarningValidation ```
|
New behavior of nested type names in Xaml... Although the dnSpy XAML result will not be directly usable by the MS toolset, it has been deemed wise not to expose the unsanctioned XML-identifier character
|
|
By the way, not sure what the "unorthodox markup extension" is that was causing a the problem in 4522338, but you might want to re-check if your fix from last week is still needed after my changes, since it seemed like some of the corruption I repair here was able to spread beyond the immediate BAML elements that were directly affected, in a few cases. I wasn't able to hit a breakpoint on your new code at all while running through hundreds of BAML-to-XAML decompliations |
My fix was for a rare case of a special markup extension. The code was Your changes look good to me now so I will go ahead and merge now! |
Super clever; I would have been tricked into just dismissing it as aberrant and moving on. As an aside, despite what everybody says, |
The current decompilation as
I'm not exactly sure what you mean here. |
|
I'm talking about the Its use is discouraged in WPF, where it does work in some cases, but it seems to be fine in general for non-WPF Xaml usage. I've been assuming that dnSpy doesn't do the special handling that would be necessary to represent the various effects of co-identifying the duplicated instance? |
|
Oh wait nevermind, I guess you're saying that dnSpy doesn't need to care about the runtime effects of any MarkupExtension (or Xaml-instantiated entity in general) in order to re-generate the source Xaml? Ok, perhaps true for In many cases, the |
Yes, dnSpy does not need to know or care about what a MarkupExtension does during to properly generate XAML from its compiled form (BAML).
These interfaces are not really part of any markup extension and the IDs used for lookup are stored in a special BAML record. The ID is added to the XElement when the XML model is being built and then during a later post processing step the IL of the Connect methods is analyzed to map the IDs to the event and handler names.
The code which is responsible for detecting the conditions inside the Connect methods is currently quite buggy and I’ve already started to look into rewriting the part which handles the case when the method does not contain a switch statement but rather a series of if statements (the buggy case). I would tell you more about my new approach to this but I’m currently on holiday without access to my PC (typing this out on a mobile device :p). The examples of the current implementation not sufficing will definitely be helpful :D |
|
Ok, so we are conflicting then, so I will stop what I'm doing for now; the football is yours. The deepest point of the false-positive rejection for this bug is here: dnSpy/Extensions/dnSpy.BamlDecompiler/Rewrite/ConnectionIdRewritePass.cs Lines 335 to 337 in 309952b |
|
@glenn-slayden I partially rewrote the algorithm used to extract the connection ID information from the if-else structures found in the |
|
TL;DR Thanks. Wow super nice job! Looks like the issue I mentioned above (namely, corrupted BAML result when browsing e.g., The left side is before your fix and the right side after:
Notice the green inline error message on is now gone, and instead there are now three new I haven't studied your changes in detail yet. Did it turn out to be what we suspected? My crude analysis was that, in some cases, the IL extracted from the But just a few hard-coded scenarios should have remained sufficient to cover any realistic number of connection ID that might happen, especially if one expects the My main hunch was that the bug would be related to when Anyway, I was starting to suspect that, in the |
|
Hi, the issue you were facing was caused by two major problems in the connection ID extraction.
Hope this somewhat explains the changes made. I've also started to work on improving the way markup extensions are handled in the decompiler to prevent issues with incorrect escaped/unescaped characters and correct handling of whitespace in parameters to markup extensions which should hopefully resolve some more issues with the decompiled XAML not being compilable. EDIT: I improved the algorithm slightly in commit 675b102, could you test it too to see just to make sure I have not introduced any regressions? EDIT2: The changes have now been turned into a PR #197 |
|
Ok, I should be able to sync up to your latest within the next couple days and I'll let you know if I see any problems. |
@glenn-slayden Any news on this? I would like to get the PR merged in the coming days :P |
|
@ElektroKill Sorry for the delay, I will try to take a look within the next 24 hours.. |
|
Ok, ran through my ad-hoc BAML test suite again with your latest change and it looks great, no problems that I can see. No "green in-line error messages" appear in the decompiled BAML. NIce job! |
Great to hear that! I will go ahead and merge the PR now :p |
1.) BAML examples containing nested type names in MarkupExtensions,now much improved:
Use
+when decompiling from BAML to XAML when some MarkupExtensions contain nested Type names e.g."{x:Static sad:WorkflowDesignerIcons+DesignerItems.WarningValidation}". The plus symbol is what the XAML compiler will accept, so I matched the behavior somewhat, but unfortunately, 'plus' is not a legal character in an XML name, when bare (unquoted) as I have currently done...BEFORE...
The use of several nested types causes rampant corruption in
System.Activities.Presentation.View.TypeBrowser:AFTER...
There are still serious problems with nested types support in XAML, but I was able to fix the major problems:
2.) Fix BAML→XAML corruption due to incorrect MarkupExtension re-expansion
BEFORE... (ToolboxDefaultTemplate.baml in System.Activities.Presentation.g.resources...)
Notice above that the "Resources" property accepting a MarkupExtension was attempted to be inlined, but this decision was faulty and led to the total corruption as shown. There were about three bugs affecting the decision conditions of whether it can be done. Most seriously, you cannout promote a MarkupExtension to inline if there is a XAML directive (i.e.
x:) attribute on the instance itself (and recursively inward), because directives are not actual attributes and would not properly understood by the MarkupExtension syntax processing. The exisiting code seemed to be aware of this idea, but it was implemented incorrectly, and also had a logic-polarity bug whereby it was ignoring most of its own efforts.AFTER...