root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.62k stars 1.25k forks source link

Cling fails to resolve ADL properly #8370

Open mramospe opened 3 years ago

mramospe commented 3 years ago

It seems ROOT is not able to resolve ADL properly. The following snippet fails to run with ROOT:

#include <iostream>
#include <any>

namespace ns {
  struct X {
    friend bool any(X) {
      return true;
    }
  };
}

void test() {
  std::cout << any(ns::X{}) << std::endl;
}

int main() {
  test();
  return 0;
}

It compiles fine with g++ -o test test.cpp but leads to the following error if run with root -l -q test.cpp:

In file included from input_line_117:1:
test.cpp:13:13: error: invalid operands to binary expression ('std::ostream' (aka 'basic_ostream<char>') and 'std::any')
  std::cout << any(ns::X{}) << std::endl;

If the header <any> is not included, then it is executed correctly.

To reproduce the error use Centos 7, ROOT version 6.24 (taken from cvmfs). This error is currently causing some developments for the LHCb experiment to be on stand-by.

eguiraud commented 3 years ago

@Axel-Naumann is this because cling injects a using namespace std? https://godbolt.org/z/9KPxPcYEr

Axel-Naumann commented 3 years ago

Thanks. Since when does it fail?

Axel-Naumann commented 3 years ago

This error is currently causing some developments for the LHCb experiment to be on stand-by.

I don't know what to suggest here, honestly. We cannot remove the interpreter's using namespace std - the only thing we could possibly do is to replace it with a generous set of using std::foos. Even then, any might be part of that set.

How attached are you to that function name? (Think of ns::string or ns::cout - most people would probably argue that these are "dangerous" names.) Changing it to anything else would work around the issue and allow some of LHCb's developments to progress.

hassec commented 3 years ago

@Axel-Naumann thanks having a look at this.

This is new code, so I guess so far we have just been lucky that we haven't created a similar conflict.

While a bit annoying, I think you are right. The most pragmatic workaround will be for us to rename that function.

If you don't mind, I've 2 follow-up questions on this.

mramospe commented 3 years ago

@Axel-Naumann by This error is currently causing some developments for the LHCb experiment to be on stand-by I just wanted to point out that this is a problem affecting the software developments of one of the LHC experiments (just in case you increase the priority of them rather than of other general issues).

I also agree that the most straight-forward solution to us is changing the names of those functions. However, I have been always surprised that the ROOT interpreter includes the "dangerous" using namespace std; line, and I wonder if there is a way to avoid that (like via an instruction that can be passed to the interpreter).

An important note is that the example above does not entirely reproduce the error that we see. We have run into a situation where root -l -q test.cpp+ compiles and runs fine (also with g++) but root -l -q test.cpp doesn't, even in the situation where we include the line using namespace std;. I am currently struggling to create a more accurate minimal working example.

Axel-Naumann commented 3 years ago

Thanks for being willing to help here - I do understand the pain...

We have thousands and thousands of user C++ files (macros) that rely on ROOT's using namespace std. We have that since the birth of ROOT, and we intentionally implemented it again for cling / ROOT6. We can do "The Right Thing" and annoy the hell out of the vast majority of our users, or have those people who are annoyed by this (and thus by definition have some expertise in coding) work around this "feature" of ROOT. Both are bad options, but IMHO the first is worse. As I said, we're considering replacing it with a bunch of using std::cout etc - but that might not help you, if std::any is part of the list. ROOT's using namespace std is even required for some (at least old) files to be read, so its suppression has more consequences than just at the prompt.

We don't have a feature to "dump cling input" - because much of cling's input is actually synthetic AST, and code that gets unloaded again, or code that is injected without proper code context, e.g. std::vector<MyClass> might be looked up by some part of cling, the I/O, or PyROOT, and that's not valid code by itself, yet has very visible effects on cling, triggering instantiation etc. If you have an idea of how to approximate this let me know!

I'm looking forward to seeing a reproducer of the actual problem - if all fails, just tell me how to reproduce it within LHCb's software stack, as far as I can use it not being a member of LHCb.

mramospe commented 3 years ago

@Axel-Naumann I understand your pain too. Applying this kind of fixes when everybody else in the world have their codes adapted to the old ROOT (and completely relying on using namespace std;) is a source of troubles and a showstopper for breaking backwards compatibility. I was wondering, however, if in ROOT 7 you would be including this kind of modifications since you are breaking backwards compatibility there.

GerhardRaven commented 3 years ago

Another option is to provide an explicit 'opt out' for those that 'want to do the right thing' -- eg. for environments where consistency with other compilers matters more than the backwards compatible behavior.

Axel-Naumann commented 3 years ago

Yes we had discussed that in the past root --pedantic :-) What's your favorite way of selecting this mode? I.e. how do we enable this for @GerhardRaven and @mramospe but not for the regular LHCb physicist using ROOT from LHCb's software stack to do their plots?

I can think of: if you use root.exe or import ROOT you get using namespace std, unless you turn it off with root.exe --pedantic or import ROOT-pedantic or whatever. If you just link against libCore you have the pedantic mode.

Axel-Naumann commented 3 years ago

We don't break b/w comp with ROOT7 - we are stopping development lines, replacing features with new versions that provide no backward compatibility (because they are new), but do provide transitioning help. Think RNTuple, TTree successor: incompatible interfaces, but tools to migrate a TTree stored in a ROOT file to a RNTuple.