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

deprecate Object.factory #14681

Merged
merged 1 commit into from
Dec 10, 2022
Merged

Conversation

WalterBright
Copy link
Member

For discussion, see:

  1. https://issues.dlang.org/show_bug.cgi?id=16423
  2. https://www.digitalmars.com/d/archives/digitalmars/D/ModuleInfo_Object.localClasses_and_Object.find_-_any_users_366643.html

The language has problems guessing which classes should go into the ModuleInfo, and people don't want more annotations for this. The pragmatic solution is for users to make the list themselves of which classes can be instantiated with a factory.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14681"

@WalterBright WalterBright force-pushed the deprecateObjectFactory branch 4 times, most recently from c29c5c9 to 44eed91 Compare December 10, 2022 06:14
druntime/src/object.d Outdated Show resolved Hide resolved
@dlang-bot dlang-bot merged commit 5600cfb into dlang:master Dec 10, 2022
@WalterBright WalterBright deleted the deprecateObjectFactory branch December 10, 2022 23:13
@PetarKirov
Copy link
Member

@dkorpel @thewilsonator I'm not sure if @WalterBright's intention was to merge this straight away. I think he wanted to see how this fares on BuildKite. @WalterBright since this PR is merged, I think you should either follow up with a changelog entry, along with deprecation messages and dates for, or revert the change, in case it was just an experiment.

@Geod24
Copy link
Member

Geod24 commented Dec 12, 2022

Yeah, this shouldn't have been merged without:

  • A deprecation message on Object.factory;
  • A changelog entry;

@thewilsonator this isn't a trivial change.

@schveiguy
Copy link
Member

Yes, I had thought this was not ready for inclusion yet. Object.factory is an obscure feature that has been broken for years, and we should be OK deprecating it, but it needs to be explained better in the deprecation.

In particular, how to replace the feature with a compile-time registration would be warranted, along with an explanation that Object.factory was unreliable in the first place.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 29, 2023

@puneet
Copy link
Contributor

puneet commented Feb 12, 2023

I did not come across this PR until last week.

I use object.factory extensively. I have an implementation of IEEE 1800.2 UVM standard implemented in D Language. The UVM standard mandates that the test to be run be implemented as a class and can be specified from the command line when running the test. As a result, I need to create an object of the test class from the given string and Object.factory works right out of the box for my use case.

Earlier I had tried implementing the same functionality using a class registration mechanism that used a static constructor for the class to be registered. Soon I found myself struggling with cyclic dependencies in the code modules.

I request Object.factory be retained until we find a more robust mechanism to resolve cyclic dependencies in modules when static constructors are used.

@WalterBright
Copy link
Member Author

This PR broke the testsuite.

How could it break the test suite when it passed all the tests before being merged?

@WalterBright
Copy link
Member Author

@puneet the way to deal with cyclical imports A imports B and B imports A, is have both A and B import C, and put the static constructor in C.

@schveiguy
Copy link
Member

How is C supposed to register classes for factory creation from A and B?

@WalterBright
Copy link
Member Author

  1. By declaring the classes to be registered in C.
  2. I don't have the math to prove it, but I'm pretty sure that any A<=>B can be replaced with A<=C and B<=C. In fact, I think Go requires it (does not allow cyclical imports).
  3. A bit kludgier way is for the C static constructor to call extern(C) void Actor(); and extern(C) void Bctor(); and have A provide a definition for Actor and B provide a definition for Bctor.
  4. declare the constructors with pragma(crt_constructor). These are not order dependent, but one must take into account that they'll be run by the C startup code before druntime is initialized, meaning no GC will be available.

@schveiguy
Copy link
Member

Keep in mind this is to replace a feature that "just works" at the moment. If possible, it should be easy to do, and not require too much extra machinery or rearrangement of declarations. Sure, you can move all classes into one module, and then you don't need to have circular imports. But then, of course, you have lost all the structure of your project.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 16, 2023

This PR broke the testsuite.

How could it break the test suite when it passed all the tests before being merged?

I'm really sorry that dmd's testsuite (specifically druntime's) chooses to ignore compile time warnings/errors when building tests.

@WalterBright
Copy link
Member Author

Sure, you can move all classes into one module, and then you don't need to have circular imports.

I didn't say all, and it doesn't have to be one module. The thing about two modules running static constructors, which one comes first? The inheritance hierarchy provides a clear resolution. But circular imports don't. How should it "just work"?

I'm really sorry that dmd's testsuite (specifically druntime's) chooses to ignore compile time warnings/errors when building tests.

The compiler still should compile and run deprecations/warnings successfully.

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

Successfully merging this pull request may close these issues.

9 participants