Mvvmcross: Commit d2a7fb2d on June 15 breaks compatibility with PropertyChanged.Fody

[borbmizzet]

🔙 Regression

IMvxNotifyPropertyChanged.RaisePropertyChanged was changed from void to Task in commit d2a7fb2d on June 15, breaking compatibility with PropertyChanged.Fody.

Old (and correct) behavior

MvvmCross played nice with PropertyChanged.Fody without crashing up through Mvx6.1.2

Current behavior

Combining PropertyChanged.Fody with Mvx6.2-beta results in crashing with System.InvalidProgramException: Invalid IL code in MyApp.Core.ViewModels.LoginViewModel:set_RememberMe (bool): IL_0020: ret

Reproduction steps

Use PropertyChanged.Fody with auto-properties and Mvx.Forms 6.2.0-beta

Configuration

Version: 6.2.0-beta2/6.2.0-beta1

Platform:

• [ ] :iphone: iOS
• [ ] :robot: Android
• [ ] :checkered_flag: WPF
• [ ] :earth_americas: UWP
• [ ] :apple: MacOS
• [ ] :tv: tvOS
• [x] :monkey: Xamarin.Forms

[nickrandolph]

@borbmizzet is Fody looking for something specific, or just bombing out of the IL that's generated? This sounds like a limitation of Fody more than a MvvmCross issue. Are you able to raise this issue on the Fody repo and see if they can investigate why it's breaking?

[borbmizzet]

@nickrandolph It's boming out of the IL at runtime when doing the bindings of the first viewmodel. I'm not 100% sure, but I believe its because fody is inserting IL code that assumes and thus requires that RaisePropertyChanged returns void. Personally, I think making it a task was a good thing overall, so perhaps what could be done for now is to have something like void LegacyRaisePropertyChanged so that at the very least people can add that in FodyWeavers.xml as

<Weavers>
  <PropertyChanged EventInvokerNames="LegacyRaisePropertyChanged"/>
</Weavers>

to keep projects using fody functional until a better solution can be implemented in either fody.propertychanged or Mvx.

[borbmizzet]

I pushed a PR to https://github.com/Fody/PropertyChanged/pull/346 to fix this problem.

[nickrandolph]

You happy for us to close this issue then?

[borbmizzet]

@nickrandolph yeah, but I'd avoid pushing out 6.2 until that PR is merged in and released to Fody/PropertyChanged so you don't get a massive influx of duplicate issues when developers that use PropertyChanged.Fody bump their Mvx version and end up breaking all the things.