Make calls recognize associations #536
Conversation
There was a problem hiding this comment.
Thanks for all your work on this @JosephWood2001, this is a really useful addition. I have a few comments, but they're all pretty minor
ford/sourceform.py
Outdated
| """ | ||
| adds a batch of associations to the associations dictionary | ||
| """ | ||
| self._batches.append({}) |
There was a problem hiding this comment.
I suggest flipping this round a little bit, doing something like:
current_batch = {}
# Fill current batch
...
self._batches.append(current_batch)This just saves the mental overhead of parsing self._batches[-1] all the time
ford/sourceform.py
Outdated
| self._batches.append({}) | ||
| for item in associations: | ||
| # parse the association | ||
| item = item.split("=>") |
There was a problem hiding this comment.
This might be clearer as:
| item = item.split("=>") | |
| new, old = item.split("=>") |
Combined with the above, we remove lots of the indexing in favour of clearer names
ford/sourceform.py
Outdated
| if k not in self._dict: | ||
| self._dict[k] = [] |
There was a problem hiding this comment.
We can change self._dict into a collections.defaultdict(list) and drop these two lines
ford/sourceform.py
Outdated
| """ | ||
|
|
||
| def __init__(self): | ||
| self._dict: Dict[str, List[List[str]]] = {} |
There was a problem hiding this comment.
I can't quite work out why the nested list -- please could you clarify?
There was a problem hiding this comment.
the inner list represents a call chain, the outer list represents all the call chains mapped to this items key, in order that they've been mapped. Multiple call chains can be mapped to one key. i.e.:
ASSOCIATION(tmp => a%b)
ASSOCIATION(tmp => c%d)
call tmp
END ASSOCIATION
call tmp
END ASSOCIATION
in this example, the first call tmp should map to the most recent association, call c%d. After we leave this association, we should still remember the previous association, and call tmp should map to call a%b
It's basically another way of representing the list of batches (List[Dict[str, List[str]]] => Dict[str, List[List[str]]])
and by having both it improves the time complexity. Looking back at this, this is silly, as the count of Batches is obviously negligible. I've rewritten it without the unnecessary _dict.
ford/sourceform.py
Outdated
| # Not raising an error here as too much possibility that something | ||
| # has been misidentified as a function call | ||
| if not hasattr(self, "calls"): | ||
| return |
There was a problem hiding this comment.
Please could you restore this bit? I know it's not technically needed, but it silences a couple of mypy errors
ford/sourceform.py
Outdated
| if context is None: | ||
| # failed to find context | ||
| return None |
There was a problem hiding this comment.
Maybe this should call self.print_error with a warning message as well?
There was a problem hiding this comment.
I'm unsure about this. Not finding the context just means that any of the contexts leading up to the call aren't registered in Ford. This could simply be because the context is from an external module, and could be quite common, and expected in some codebases.
ford/sourceform.py
Outdated
| This is done by looking at the first label in the call chain and matching it to | ||
| a variable or function in the current scope. Then, traverse to the context of the | ||
| variable or function return and repeat until the call chain is exhausted. | ||
| a variable, function ext. in the current scope. Then, switch to the context of |
ford/sourceform.py
Outdated
| if call_type is None: | ||
| return None | ||
| context = self | ||
| for i, call in enumerate(call_chain): |
There was a problem hiding this comment.
Nice, this whole bit is much clearer!
ford/sourceform.py
Outdated
|
|
||
| # Register the associations | ||
| assoc_str = ford.utils.strip_paren( | ||
| self.ASSOCIATE_RE.match(line).group(2), 0 |
There was a problem hiding this comment.
You could make this even better by using match := self.ASSOCIATE_RE.match(line) in the elif so we can reuse match, and then also naming the group in ASSOCIATE_RE so we can do match[<group name>] here
ford/sourceform.py
Outdated
| if i == len(call_chain) - 1: | ||
| return item |
There was a problem hiding this comment.
Consider rewriting the loop to just be over call_chain[:-1]?
|
👍 applied suggestions, except for adding a warning to contexts that can't be found. |
refactors the call chain correlation to make it much simpler, and also adds some functionality to allow it to parse call chains that may have been modified by an association (association modified call chains might have a function call in the middle of the call chain)
recognizes associations and applies them to call chains. Additionally, the rhs of associations are called when the association is made, and these are now also added as call chains.