search

art-framework-suite / art

The implementation of the art physics event processing framework
Other
2 stars 7 forks source link

make_tool does not check type #88

Open knoepfel opened 3 years ago

knoepfel commented 3 years ago

This issue has been migrated from https://cdcvs.fnal.gov/redmine/issues/19674 (FNAL account required) Originally created by @dladams on 2018-04-13 13:51:12

I find that art::make_tool(myParamSet) returns a pointer to the object described by myParamSet for any Type1, i.e. there is no type checking. This leads to undesirable behavior including crashes if the object is not really Type1.

It would be preferable for make_tool to return null or raise an exception if the object is not of type Type1.

An example may be found in dunetpc/dune/ArtSupport/test/test_make_tool.cxx.

knoepfel commented 3 years ago

Comment by @knoepfel on 2018-04-16 16:39:41

What you describe is correct. In order to provide the type-checking desired, additional constraints/prescriptions are necessary for the user to adopt. We will describe in more detail what the issues are in a future message.

knoepfel commented 3 years ago

Comment by @knoepfel on 2018-04-17 14:33:30

Whenever we load libraries via our own plugin manager, it is necessary to perform a hard cast on the extern C function that creates the plugin. So when you call art::make_tool<MyInterface>(ps), the following steps are performed:

It is the hard cast that erases any type safety.

This has not been a problem in the past since art is usually responsible for creating the plugins, and we make sure to avoid the problem by providing the correct types. However, since make_tool is intended to be user-facing, there are additional responsibilities that users must take to avoid this problem.

There are two options we can pursue:

  1. Keep things how they currently are, with the expectation that authors invoking make_tool must make sure that the correct types are specified for any given allowed ParameterSet configuration.
  2. Add additional interface that the user must invoke when defining the tool. The interface would be akin to the implementer/interface API that exists for services. Although more work for the user, it has the benefit of avoiding any disastrous failures similar to the ones you've encountered.

If you'd like to pursue option 2, then we would need to discuss this with the stakeholders to get agreement and determine priority. Please let us know.

knoepfel commented 3 years ago

Comment by @dladams on 2018-04-17 15:09:28

Kyle:

Thanks for working on this. Can we do both, i.e. keep the current interface so we don't break any existing code and provide a second type-safe interface?

For 2 are you talking about decorating the interface header with a base class and/or macro and adding dependency on art/canvas? I find it very attractive now that it has no such dependency or decoration and would prefer a solution that avoids those.

If it makes things easier, I would happily settle for a solution that only works for class tools whose interfaces have virtual methods.

knoepfel commented 3 years ago

Comment by @knoepfel on 2018-04-23 16:29:13

Our preference is to provide one or the other, however we will discuss the pros and cons of either approach at the next stakeholders meeting. We are also amenable to deprecating the type-unsafe interface for a time while users get accustomed to the type-safe interface.