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

Globally disable Pylint E0606 possibly-used-before-assignment rule + fix one case #1349

Merged
merged 2 commits into from
May 15, 2024

Conversation

matt-graham
Copy link
Collaborator

As noted in #1098 (comment), the latest v3.2.0 Pylint release adds a new check possibly-used-before-assignment / E0606 which checks for possible usages of variables before they are assigned to. This however currently seems to be throwing up a lot of false positives when running checks on main:

check: commands[5]> pylint src tests
 ************* Module src.tlo.analysis.hsi_events
src/tlo/analysis/hsi_events.py:498:12: E0606: Possibly using variable 'inspect_hsi_event_details' before assignment (possibly-used-before-assignment)
src/tlo/analysis/hsi_events.py:498:39: E0606: Possibly using variable 'json_hsi_event_details' before assignment (possibly-used-before-assignment)
************* Module src.tlo.methods.pregnancy_helper_functions
src/tlo/methods/pregnancy_helper_functions.py:430:16: E0606: Possibly using variable 'risk' before assignment (possibly-used-before-assignment)
************* Module src.tlo.methods.postnatal_supervisor
src/tlo/methods/postnatal_supervisor.py:864:46: E0606: Possibly using variable 'risk_of_death' before assignment (possibly-used-before-assignment)
************* Module src.tlo.methods.pregnancy_supervisor
src/tlo/methods/pregnancy_supervisor.py:913:60: E0606: Possibly using variable 'first_anc_appt' before assignment (possibly-used-before-assignment)
************* Module src.tlo.methods.contraception
src/tlo/methods/contraception.py:698:78: E0606: Possibly using variable 'due_appt' before assignment (possibly-used-before-assignment

Of these the only one I think is potentially valid is src/tlo/analysis/hsi_events.py:498:39: E0606: Possibly using variable 'json_hsi_event_details' before assignment (possibly-used-before-assignment) as in

if args.json_file is not None:
with open(args.json_file, 'r') as f:
hsi_event_details = json.load(f)
# JSON serializes tuples to lists therefore need to reformat to reconstruct
# HSIEventDetails named tuples
def recursive_list_to_tuple(obj):
if isinstance(obj, list):
return tuple(recursive_list_to_tuple(child) for child in obj)
else:
return obj
json_hsi_event_details = set(
HSIEventDetails(
**{
key: recursive_list_to_tuple(value)
for key, value in event_details.items()
}
)
for event_details in hsi_event_details
)
print(f'HSI events loaded from JSON file {args.json_file}.')
if args.json_file is None or args.merge_json_details:
print('Getting details of defined HSI events by inspecting tlo.methods...')
inspect_hsi_event_details = get_details_of_defined_hsi_events()
print('...done.\n')
if args.merge_json_details:
hsi_event_details = merge_hsi_event_details(
inspect_hsi_event_details, json_hsi_event_details
)

if args.json_file is None and args.merge_json_details is True then line 498 will be run and json_hsi_event_details will not be defined (this set of arguments would not in itself make sense but we should probably still have a more informative error message triggered if these arguments are encountered).

The other cases I think are all false positives, corresponding to variants of the following minimum working examples

def case_a(condition_1: bool, condition_2: bool):
    if not condition_1 and condition_2:
        condition_3 = True
    return condition_2 and (condition_1 or condition_3)


def case_b(condition: bool, key: str):

    def inner_function(key):
        a_dict[key] = 2

    if condition:
        a_dict = {key: 1}
        inner_function(key)


def case_c(condition_1: bool, condition_2: bool, number: float):
    if condition_1:
        a_variable = 1
    elif condition_2:
        a_variable = 2
    if condition_1 or condition_2:
        if number < a_variable:
            ...
    ...


def case_d(condition_1: bool, condition_2: bool):
    if condition_1 or condition_2:
        variable = ...
    if condition_2:
        print(variable)

with Pylint flagging E0606 failures on all four case_* functions even though I think in all cases it is not possible for code path with an undefined variable to be reached. There are already a couple of open issues on Pylint repository pylint-dev/pylint/issues/9628 and pylint-dev/pylint/issues/9627 with regards to false positives with E0606, and I think some of above cases may suggest there are further things needing fixing to make this check robust.

This PR therefore globally disables the Pylint E0606 rule by adding to the tool.pylint.main.disable list. It also adds logic to raise an exception with a more informative error message for the true positive identified in hsi_events.py with regards to json_hsi_event_details potentially not being defined when used for some (invalid) argument values, though because of I think pylint-dev/pylint/issues/9627 a false positive E0606 violation is still registered after this fix.

@matt-graham matt-graham requested a review from tamuri May 15, 2024 13:25
@matt-graham matt-graham mentioned this pull request May 15, 2024
20 tasks
@matt-graham matt-graham merged commit f7cf84c into master May 15, 2024
56 checks passed
@matt-graham matt-graham deleted the mmg/disable-pylint-e0606 branch May 15, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants