Skip to content
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

[BUG] Possible interoperability problem between .NET Standard 2.1 packages and .NET 7 projects #96

Closed
jodydonetti opened this issue Dec 10, 2022 · 16 comments

Comments

@jodydonetti
Copy link
Contributor

Hi @neuecc , and thanks for creating MemoryPack, it's damn good!

A user of my lib has found a runtime problem while using my library, which is a .NET Standard 2.1 package that uses MemoryPack.

What I think is happening here is that I compiled my library while targeting .NET Standard 2.1, since that is a valid target TFM for MemoryPack: this means that the generator did run with that TFM.

But then the user referenced my .NET Standard 2.1 library from a .NET 7 project, and here is where my theory for the problem is: I think that the code you use at runtime is strictly related to the TFM used by the generator, meaning that if a project is targeting .NET 7, than all the code that your generator creates must also target .NET 7.
But the problem is that your generator may have already run for other classes in a referenced package, that targeted .NET Standard 2.1 (which is also valid, and a subset of .NET 7).

By having .NET Standard 2.1 as a valid target you are basically saying that anyone can use the MemoryPack generator and create a package that targets .NET Standard 2.1. But someone else may use that package (which contains the generated code) and use that with a .NET 7 project (which is a superset of .NET Standard 2.1), and this seems to break things.

Is my analysis correct?

Thanks!

@jodydonetti
Copy link
Contributor Author

Hi @neuecc ,
yes I remember that, but then I asked:

do I also have to let the generator generate the code and grab the code twice?

to which you answered:

Yes!
Ah...sorry, the main difference, however, is the use of static abstract members.

but also added:

In the generation of external formatters, even .net7 does not use it, so
Perhaps the same code may be sufficient.

So I went and used the same code.
Now I can of course make the needed change now, which I will probably do immediately to fix this, but I wanted to share my thoughts about the next part:

This is documented.
https://github.com/Cysharp/MemoryPack#target-framework-dependency

I missed that part, sorry.
Having said that, my impression here is that this approach (although documented) breaks the official and known .net contract about different versions compatibility: what I mean is that when we say "a lib supports .net std 2.1" what we mean should be that it will work on any compatible implementation of such standard, like .net 6 and .net 7.

Here instead the design is such that if my lib targets .net std 2.1 and uses your lib (which also targets .net std 2.1), it would still not work on a project that targets a version of .net that is compatible with .net std 2.1.

This is the first time I saw such approach and I think it breaks the expectations of the users.

If you feel like, I would gladly chat about this in more details via DMs or a videocall.

@neuecc
Copy link
Member

neuecc commented Dec 11, 2022

Oh, sorry, some of my responses were misleading and incorrect.
As for compatibility, the ReadMe description is correct.

This is the first time I saw such approach and I think it breaks the expectations of the users

Yes. However, static abstract members are the first significant runtime-incompatible change in a long time (like Net 1 -> 2 Generics for that matter).
Currently, the only 3rd party library that utilizes static abstract members is MemoryPack, so you will not encounter any problems.
I am interested to see how others handle this.

Not using static abstract members for compatibility can not choose in this library, for ultimate performance with current .NET.

@jodydonetti
Copy link
Contributor Author

jodydonetti commented Dec 11, 2022

This is the first time I saw such approach and I think it breaks the expectations of the users

Yes. However, static abstract members are the first significant runtime-incompatible change in a long time (like Net 1 -> 2 Generics for that matter). Currently, the only 3rd party library that utilizes static abstract members is MemoryPack, so you will not encounter any problems. I am interested to see how others handle this.

Me too! It's a delicate subject and we'll see how it works out in the field with other projects.

Not using static abstract members for compatibility can not choose in this library, for ultimate performance with current .NET.

Got it, makes sense.

Would it be possible though to have them work, like, side-by-side with the standard approach? What I mean is that the generated code for the .net std 2.1 version would work even when running in a .net 7 project? Otherwise as I pointed out the promise of .net std 2.1 <-> .net 7 compatibility would be broken, which is a big surprise for users.

I don't know the intricacies of the MemoryPack codebase right now, but by doing some mental reverse-engineering I think the main issue is that internally you are using 2 very different workflows for .net std 2.1 and .net 7, which are not compatible with each other. Maybe a unified workflow with an automatic runtime opt-in switch would be possible? Something like the .net 7 workflow being able to also support a .net std 2.1 formatter or something like this.

Thanks for your support.

PS: the combo of "new runtime features" + "source generators" we are seeing here for the very first time may highlight a design detail never happened before, and it may lead to some new guidelines for source generator users. It will be interesting to see how it'll go.

@jodydonetti
Copy link
Contributor Author

jodydonetti commented Dec 11, 2022

Btw, an update: in the meantime I've updated my lib to explicitly use multi-targeting, and I've switched from grabbing the generated code and copying it manually into a class (previous approach) to the other approach you suggested, which uses external formatters. Now it allocates more (since it has to allocate the serializable class) but it seems to "technically" work.
I now have some of my 300+ tests failing (only for MemoryPack) so I'm trying to understand if I made some mistakes in the formatter configuration or registration part, maybe I'm missing something.

@jodydonetti
Copy link
Contributor Author

Maybe @mgravell , who seems to be experimenting a lot with source generators, can give us some opinions?

@neuecc neuecc closed this as completed Dec 18, 2022
@jodydonetti
Copy link
Contributor Author

Hi @neuecc , what is changed?

@neuecc
Copy link
Member

neuecc commented Dec 18, 2022

It seems to have been released and I saw no reason to continue to keep this Issue.

@jodydonetti
Copy link
Contributor Author

jodydonetti commented Dec 19, 2022

Hi @neuecc , the issue on my repo was the one to track and solve my users' issue with FusionCache + MemoryPack + .NET 7.

This issue instead was meant to discuss the non-standard dependency behaviour of MemoryPack regarding .NET Standard 2.1 and .NET 7.

Recap

To quickly recap the issue I think we have here:

  1. MemoryPack targets both .NET Standard 2.1 and .NET 7, so it is saying it supports ANY of them, not BOTH of them TOGETHER
  2. any .NET Standard 2.1 package should work with any concrete .NET version that is compatible with such 2.1 standard version (which includes .NET 3.0, 3.1, 5.0, 6.0 and 7.0)
  3. currently this is not the case, and the dependency is broken, since any package working with MemoryPack MUST multi-target to .NET 7 explicitely to be able to function, which is non-standard behaviour

Example

Practical example:

  • I create a package PackageA which targets .NET Standard 2.1 and uses MemoryPack
  • I create a project ProjectB which targets .NET 7 (which implements .NET Standard 2.1) and references my package PackageA (which also targets .NET Standard 2.1)
  • this should work, but currently does not

The practical issue is that currently MemoryPack, when it sees it is running in .NET 7, expect all the dependency chain to be .NET 7, as is stated on the repo's README:

.NET 7 project shouldn't use the netstandard 2.1 dll. In other words, if the Application is a .NET 7 Project, all the dependencies that use MemoryPack must support .NET 7. So if a library developer has a dependency on MemoryPack, you need to configure dual target framework.

This, even though is stated in the README, is still a behaviour that violates the standard compatibility rules of .NET as a whole, and breaks the users' expectations about compatibility.

As an additional prove that this violates the .NET compatibility rules, this is my package previous version (which was not working well in a .NET 7 runtime) on the Nuget site where, based on the declared target frameworks, renders like this:

image

As you can see, by applying the standard dependency logic, the Nuget website (and the entire ecosystem of tools, compilers, plugins, etc) is saying that my package explicitly targets .NET Standard 2.1 (dark blue) but is also compatible and can run on all the others shown .NET versions (light blue), including .NET 7.

And that was not the case, because of the above issue.

It's your project

Now to be clear: of course the project is yours, and you can decide this is the way to go!
But I opened this issue to discuss the subject and maybe find other options, since I think it will hurt your project's viability in a lot of scenarios (and I would really like MemoryPack to succeds!).

A possible solution

My 2 cents about a solution (admittedly without knowing MemoryPack codebase that much): would it be possible for MemoryPack, when running on .NET 7, to be able to work with code generated by the source generator in both a .NET Standard 2.1 AND .NET 7 versions? Meaning that to get the full blown perf you must have all the formatters generated on .NET 7, ok, but it would still be possible for it to work with .NET Standard 2.1 formatters, just in a less performant way?

This would:

  1. solve the non-standard issue
  2. allow to have mixed formatters (some .NET 7, some .NET Standard 2.1), and let users get their job done without surprising exceptions
  3. still allow the max possible perf boost, in case all formatters are actually compiled against .NET 7

An additional idea may be to log a message that warns about a possible perf boost opportunity, in case it detects a .NET 7 runtime with some .NET Standard 2.1 formatters, to ease the experience for new users and highlight a path to success.

Again just my 2 cents, since I would really like MemoryPack to win big in this space 😊

ps: thanks again for your openness and support, it's not a given!

@neuecc
Copy link
Member

neuecc commented Dec 19, 2022

This is not a Source Generator issue, but an interface definition issue, because we are using different interfaces (although the names are the same).
.NET 7 and beyond rather than compatibility.

@jodydonetti
Copy link
Contributor Author

Hi @neuecc , which interface (definition) are you talking about? I'm not understanding, sorry.

Basically though, as I tried to clarify above, the main point is this: currently the chain (MemoryPack -> NS 2.1 LIB -> N7 APP) is broken, when instead it should work, and this is against the .NET compatibility guidelines and every supporting tool out there (like Nuget itself).

I think it would be better to change the approach as I described above, but of course this is your project so it's up to you.

I just hope I've been able to explain myself, maybe something has been lost in translation.

Thanks!

@neuecc
Copy link
Member

neuecc commented Dec 22, 2022

Obviously, the definition of interface is different, isn't it?
https://github.com/Cysharp/MemoryPack/blob/main/src/MemoryPack.Core/IMemoryPackable.cs

I have no content to discuss.
If there is a good alternative, I will adopt it, and if there is not, there is not.

@jodydonetti
Copy link
Contributor Author

Hi, I opened this issue to highlight that the current design violates the .NET compatibility guidelines, and to discuss possible alternatives. I didn't had a solution ready, since the codebase is quite complex.

Anyway since you want content to discuss I can try with some pseudo code (so, NOT actual code) to give you an idea of what I'm talking about.

I am thinking about interfaces like these to define formatters:

// FOR NS 2.1
public interface IFormatter {
  void Serialize(...) {
    // ...
  }
  void Deserialize(...) {
    // ...
  }
}

#if NET7_0_OR_GREATER
// FOR NET 7
public interface IFormatterNet7 : IFormatter {
  // STUFF COMPATIBLE -ONLY- WITH NET 7
  static abstract void SerializeNet7(...) {
    // ...
  }
  static abstract void DeserializeNet7(...) {
    // ...
  }
}
#endif

Please note that the static abstract methods are just an example of something tha would work only on NET 7 and above.

The generator can then generate the needed concrete formatter classes (like MyTypeFormatter for the type MyType) by always implementing the first interface (which would make it usable by any NET compatible with NS 2.1), and the second interface inside an #if NET7_0_OR_GREATER block, with the even more optimized version targeting NET 7.

Then, in the code for the MemoryPackSerializer.Serialize<T>(...) method, we would have something like this:

public static MemoryPackSerializer {
  // ...
  public static byte[] Serialize<T>(...) {
    IFormatter f = GetTheFormatterFor<T>();

#if NET7_0_OR_GREATER
    if (f is IFormatterNet7 f7) {
      f7.SerializeNet7(...);
      return;
    }
#endif

    f.Serialize(...);
    return;
  }
  // ...
}

Again, this is just a very simplified example to show you the approach.

In this way MemoryPack:

  • when running on NS 2.1 compatible runtimes (.NET 5, 6, etc) can use formatters generated for NS 2.1
  • when running on NET 7 can use formatters generated for NET 7 AND formatters generated for NS 2.1, both at the same time

Of course to get the 100% best performance someone would have to multi-target every piece of the chain, so that on NET 7 every formatter would have been generated targeting specifically NET 7, and use the code generated specifically for that.
But when referencing libs with formatters generated for NS 2.1, at least it would work (and the code on NS 2.1 is still very very fast btw).

I hope I've been able to show you the approach I'm talking about.

And of course: maybe there's a reason why an approach like this is not possible, I don't know, and therefore I'm asking.

Thanks.

@neuecc
Copy link
Member

neuecc commented Dec 23, 2022

    if (f is IFormatterNet7 f7) {
      f7.SerializeNet7(...);
      return;
    }

This is one of the methods I would like to achieve, but it is not possible with the current C#.
Will have to wait for C# 12 or 13.

@jodydonetti
Copy link
Contributor Author

Hi, sorry but I've been ill for some time now.

Probably It's my fault for not having been clear enough, sorry but language barrier or something I suppose.

but it is not possible with the current C#.

I don't know what you mean, but anyway yes, you already can (at least what I meant).

I prepared a POC (Proof Of Concept) repo here https://github.com/jodydonetti/FooPack to showcase the approach I'm talking about.

As said there, it is NOT a working serializer or anything, it's just a proof of concept for the approach. I hope the README is clear enough.

If you look at the results here you can see that it never crashes in .NET Standard 2.1 nor in .NET 7, and it always tries to use the best formatter available or it fallbacks to the standard one when a .NET 7 version is not available (so running in the .NET 7 console, it can work with the formatter generated in the .NET Standard 2.1 lib).

Hope this helps clarify my approach.

Let me know what you think about this, thanks!

@neuecc
Copy link
Member

neuecc commented Jan 1, 2023

Ah, sorry it is using formatter, not formattable, I misread it.
Formatter is used is fallback(or only at the top level), otherwise calls Packable (WritePackable).
There are performance advantages to not going through Formatter.
However, this optimization is only available in .NET 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants