CASL / Futility

VERA Fortran Utilities
Other
44 stars 20 forks source link

Do not automatically recursively search PL when full path is provided #256

Closed aarograh closed 3 years ago

aarograh commented 4 years ago

There have been instances of the wrong parameter being fetched when a full path was provided to the correct parameter, specifically due to the fact that the has method returned true when it should not have. This issue covers efforts to address this.

Two changes are suggested:

  1. Add an optional recursive argument to has and get functions. If set to true, then the current recursive search will be performed. Otherwise, only the exactly specified path will be searched
  2. Modify the has and get functions to use this recursive argument. By default, if at least one -> is present in the parameter name, a recursive search should NOT be done. If a single parameter name is provided with no path (i.e., no ->), then a recursive search will occur by default. However, the optional argument will be exposed for the client code to override these default behaviors in either case.

This will eliminate some unpredictable behavior that has been problematic for client-side code. Additionally, it is expected that eliminating the recursive searches could have non-trivial performance improvements as well.

aarograh commented 4 years ago

As I'm thinking about this more, I'm wondering if the default should be to never do a recursive search, regardless of whether a path was provided or not? There are some times where we probably want to, but with the optional argument exposed, client code can just specify those times. It would simplify the "contract" for parameter lists. Thoughts?

bscollin commented 4 years ago

Well, the biggest thing I can think of is that if I just type get('power') that isn't a direct path because the top level is always a list, not the entries on the list. I think you'd need to either do something special about that or keep it being recursive by default. Honestly, without looking, I couldn't tell you what the top level list name even is.

aarograh commented 4 years ago

Yeah, there are several different ways that top level can get set I think. I'm ok with leaving it recursive in that particular case. Just wanted some thoughts from others since I can see some pros/cons either way.

bkochuna commented 4 years ago

The current search behavior is best described as the code attempts to match passed address argument as the "terminal address in the list".

To clarify the behavior of the current code consider the following example: You have a list with the following structure:

D->
   A->
      B->C
A->
   B->C

For a search performed on the list address A->B->C It will search the entire parameter list for any address that ends with this sequence. Therefore in the above example, the code should match on D->A->B->C and not A->B->C as it performs a depth first search. I believe it could never match on the second A->B->C with its current implementation.

I'd be interested to see the case where it falsely returns true for a has argument.

bkochuna commented 4 years ago

Also another thought is we could do recursive vs non-recursive search by having the address start with -> to signify "start anywhere" or "match anywhere"/"match first"

aarograh commented 4 years ago

Brendan, see my comment on this ticket:

https://code.ornl.gov/vera/vera-dev/-/issues/3646

Technically I think the search is following the behavior you describe, but it is VERY wrong as far as what I, as the client, expect and need.

bkochuna commented 4 years ago

I would--but it looks like my ucams credentials for vminfo are not working... they worked fine Saturday, I suspect its something with it being June 1. I'll look once I get that sorted out.

bkochuna commented 4 years ago

Hey @aarograh I looked at that ticket, and that looks like a bug in ParameterLists to me. From my example its ok to match A->B->C in D->A->B->C. The code should not be matching something like A->B->C in A->B->D->C.

I'm not sure that this PR gets at that heart of the issue, but instead just works around it.

If you want I can investigate this week. Probably starting tomorrow.

aarograh commented 4 years ago

I still think there's value in making the recursive searching optional. We spend quite a lot of time searching parameter lists during input processing.

aarograh commented 4 years ago

But I'm good with you investigating the bad matching. I probably wasn't going to have time this week, and I suppose it might be an issue even if we make changes to the recursive search behavior too.

aarograh commented 4 years ago

I'm going to start chipping away at this a bit whenever I'm waiting on cases/tests to run.

So far, I've gotten a unit test in place using the results from ticket 5798. The test simply adds the relaly long path named in the ticket, then does a %has(...) check using the other shorter path. The check should return false, but incorrectly returns true. So this is a good starting point to start fixing the problem.