ADR suggestion: New code should use type hints

[rozyczko]

As discussed during the code camp (07/01/2025) we are trying to move towards having type-hints in the code.
Before addressing the bulk of the code, we should start requiring all the new code to contain type hints for all method signatures.
Checking type hints should also be a part of the code review.

[krzywon]

If we implement ruff as in #3171, this might happen automatically...

[krzywon]

For consistency we should probably use annotations on the function/method definition rather than type comments below the definition. @DrPaulSharp to create ruff PRs that modify this for both sasview and sasmodels.

[krzywon]

Here is the proposed ADR for type hinting.

Type hinting style guide

Preamble

Python is a dynamically typed programming language, so variables passed to and used in modules and classes may be of any value type at run-time. To minimize issues related to type mismatch, a base type-hinting structure is proposed.

Proposal

Adding type hints

• All new code is required to have type hints that match this proposal.
• As existing code is modified, whether a bug is fixed, or a module refactored, the developer making changes should ensure any existing code is modified to match this proposal.

Type hint structure

• In general, all type hints should follow the latest python typing scheme outlined in https://docs.python.org/3/library/typing.html.
• In general, all type hints should be as explicit as necessary to ensure developers can replicate the type(s) allowed by the module.
• Built in types should be used to minimize imports from the typing module, e.g. list instead of from typing import List.
• Types with multiple options should be separated by |, e.g. str | float | int instead of importing Union from the typing module.
• Variables that could be None should include | None as an option instead of using Optional from the typing module.
• User-defined type hints should be after all imports, but before any function, module variable, or class definitions.
• User-defined type hint variable names should be in PascalCase.
• Variables passed to functions and methods should use annotated type hints, e.g. def add(value1: float, value2: float) -> float.
• Return types should always be explicitly defined. In cases where no value is returned, use -> None
• Class variables should be annotated as the variable is defined in the __init__() method, e.g. self.var1: float = 0.0
• Top-level module variables should include type hints of the same style as class variables
• Lists, tuples, and dictionaries should use [] to define the object type(s) held within.
  • list[str] suggests a list of strings
  • tuple[int] suggests a tuple of any length, with all elements integers
  • tuple[str, int] suggests a tuple of two items, the first a string and the second an integer
  • dict[str, str | float] suggests a dictionary with all keys as strings and values either a string or a float
• Imports that are only used for type hinting can be wrapped in an if TYPE_CHECKING: block to prevent circular imports.

Type hint review process

• Pull request reviewers should not approve a request if proper type hints are not included in any new method, function and/or class definition. A review note should be added for each missing type hint and the style of type hint that should be used.
• Pull request reviewers should add a note to any pre-existing method, function and/or class that has been modified as part of a pull request and does not have proper type hints. This does not prevent a pull request from being merged, but making theses suggested changes is strongly encouraged.

[DrPaulSharp]

All of this looks good. I'd particularly emphasise the importance of using built-in types rather than the typing module. There is an automatic fix in ruff that will make this change, however this caused difficulties in #3544 as the typing module was used as a transitive import. I would strongly discourage the use of transitive imports in SasView, I think it is likely to be a cause of the considerable number of import problems I found recently.

[lucas-wilkins]

I think quite a few of the typing module uses are legacy, from the not to distant past where it was necessary.

[butlerpd]

As mentioned in the email to developers, this ADR was brought up for final approval at the October 7th meeting.

The number of people on the call at the time of this discussion was relatively small. However, those remaining raised concerns that this requires a lot more thought than is evidenced in the discussion so far. In particular, this ADR will have an immediate high impact on developers and could adversely affect the current effort to broaden the contributor base. Specific points raised:

• Contrary to the understanding of some, @DrPaulSharp indicated that Ruff will not automatically take care of this
• More useful in this regard is probably the use of IDE's with an integrated AI such as copilot though even then, one would need to check as current experience seems to be it is only ~80% correct.
  • However, it was noted that while the use of AI tools is growing, their use in this context seems to remain very limited outside those whose jobs involve a significant amount of "production" coding.
• Python has gained wide acceptance in scientific coding largely because of it simplicity for non programmer scientists. However, we seem to be on a path of eliminating the very reason for using it: democratizing scientific coding.
• On the other hand, everyone agreed that it is critical for the project's sustainability to continuously improve our coding practices and that the current type hints suggestion is clearly an important step along that path.

RECOMMENDATION:
Before final approval there should be more discussion around understanding how this ADR will affect the ability to obtain future contributions and whether it can be implemented in a way that minimizes any negative impacts.

[llimeht]

Contrary to the understanding of some, @DrPaulSharp indicated that Ruff will not automatically take care of this

Not sure which 'this' needs to be taken care of, but:

• old Python 2 style comments of which there are many can be converted into Python 3 type annotations using com2ann
• pyupgrade works well at upgrading annotations such as typing.List into list, Union into |, Optional into | None. This tool can be quite helpful in an old code-base like sasview, particularly given that we're now thinking about a minimum python version more explicitly; it would also update old style super and do some f-string updates, some of which would overlap with what ruff could also deliver.

I've had good success on largeish code bases with these before. They can also be deployed file-at-a-time to review and then commit; they tend to omit a few things that could be improved further just out of caution.

More useful in this regard is probably the use of IDE's with an integrated AI such as copilot though even then, one would need to check as current experience seems to be it is only ~80% correct.

Kind of... looking at annotations and getting them to work with mypy --strict is a great way of discovering (and then fixing) a lot of bugs. The biggest issue is that it's extremely boring work and you don't get much payback until you've done a lot of it and start seeing the inconsistencies emerge. The big benefit comes when you have enough annotations that a linting tool can go "nope, that's not right" at new code based on static analysis; now that's a long way off, but you have to eat an elephant one mouthful at a time.

Python has gained wide acceptance in scientific coding largely because of it simplicity for non programmer scientists. However, we seem to be on a path of eliminating the very reason for using it: democratizing scientific coding.

Kind of... For some of the functions in sasview, writing the annotations is hard and it would be fine to punt on those or ask someone else for help. For most it's really quite easy and if we're merging code that doesn't explain what the types are going in and out of functions, then that's technical debt that we will have to pay back later and, most likely, latent bugs.

On the other hand, everyone agreed that it is critical for the project's sustainability to continuously improve our coding practices and that the current type hints suggestion is clearly an important step along that path.

RECOMMENDATION: Before final approval there should be more discussion around understanding how this ADR will affect the ability to obtain future contributions and whether it can be implemented in a way that minimizes any negative impacts.

There is lots of "require"-level language the current text about their presence. In the spirit of doing something in this situation is better than doing nothing, is it a matter of softening the ADR just a little so that we can get continuous improvement? We might find that it's not as scary as it looks in a couple of releases and then we can revisit "should"/"please"/"recommended" and use "must"/"require".

[DrPaulSharp]

Not sure which 'this' needs to be taken care of, but:

• old Python 2 style comments of which there are many can be converted into Python 3 type annotations using com2ann
• pyupgrade works well at upgrading annotations such as typing.List into list, Union into |, Optional into | None. This tool can be quite helpful in an old code-base like sasview, particularly given that we're now thinking about a minimum python version more explicitly; it would also update old style super and do some f-string updates, some of which would overlap with what ruff could also deliver.

These points have already been covered. #3578 uses com2ann to update type comments to type hints. The pyupgrade ruleset is included in the SasView ruff ruleset, so code that uses either the pre-commit hook or runs through the CI will have these upgrades applied. In #3560 I went through the entire codebase fixing linting errors from this ruleset.

What we're looking for going forward is introducing type hints to routines that do not have any type annotations. To clarify the point about ruff, there is an ANN ruleset that considers annotations. Running this on sasview gives 8520 linting errors, but with zero automatic fixes available - i.e., resolving these linting violations requires manual developer work.

On the other hand, everyone agreed that it is critical for the project's sustainability to continuously improve our coding practices and that the current type hints suggestion is clearly an important step along that path.
RECOMMENDATION: Before final approval there should be more discussion around understanding how this ADR will affect the ability to obtain future contributions and whether it can be implemented in a way that minimizes any negative impacts.

There is lots of "require"-level language the current text about their presence. In the spirit of doing something in this situation is better than doing nothing, is it a matter of softening the ADR just a little so that we can get continuous improvement? We might find that it's not as scary as it looks in a couple of releases and then we can revisit "should"/"please"/"recommended" and use "must"/"require".

I strongly agree with the point here, I think there is a real risk in this ADR of letting "perfect" be the enemy of "good" (and potentially further than that, letting "good" be the enemy of "improved"). We need to find a way of getting the work of introducing type hints started, and hopefully find a snowball effect once we get going.

[butlerpd]

coming back to this late but just to add that I agree with @llimeht that the softening the heavy use of "require-" would probably be the thing to do. Further, I suspect you are also correct that the unknown can make it much scarier than it is?

[krzywon]

Two suggested changes from the technical meeting on Dec. 12, 2025 to soften the requirements:

• The proposed phrase All new code is required to have type hints that match this proposal in the Adding type hints section should be changed to All new code is suggested to have type hints that match this proposal.
• The Type hint review process section should be modified to have a single bullet that reads During a pull request review, if type hints are missing, the reviewer should add a comment to one line without type hints with a suggested code block on proper type hinting. The lack of type hints should not hold up a pull request that is otherwise ready to be merged.

[krzywon]

Instead of suggested use highly recommended for new code - suggested by @butlerpd

[krzywon]

Here is the final ADR proposal for type hinting.

Type hinting style guide

Preamble

Python is a dynamically typed programming language, so variables passed to and used in modules and classes may be of any value type at run-time. To minimize issues related to type mismatch, a base type-hinting structure is proposed.

Proposal

Adding type hints

• All new code is highly recommended to have type hints that match this proposal.
• As existing code is modified, whether a bug is fixed, or a module refactored, the developer making changes should make every effort to ensure that code matches this proposal.

Type hint structure

• In general, all type hints should follow the latest python typing scheme outlined in https://docs.python.org/3/library/typing.html.
• In general, all type hints should be as explicit as necessary to ensure developers can replicate the type(s) allowed by the module.
• Built in types should be used to minimize imports from the typing module, e.g. list instead of from typing import List.
• Types with multiple options should be separated by |, e.g. str | float | int instead of importing Union from the typing module.
• Variables that could be None should include | None as an option instead of using Optional from the typing module.
• User-defined type hints should be after all imports, but before any function, module variable, or class definitions.
• User-defined type hint variable names should be in PascalCase.
• Variables passed to functions and methods should use annotated type hints, e.g. def add(value1: float, value2: float) -> float.
• Return types should always be explicitly defined. In cases where no value is returned, use -> None
• Class variables should be annotated as the variable is defined in the __init__() method, e.g. self.var1: float = 0.0
• Top-level module variables should include type hints of the same style as class variables
• Lists, tuples, and dictionaries should use [] to define the object type(s) held within.
  • list[str] suggests a list of strings
  • tuple[int] suggests a tuple of any length, with all elements integers
  • tuple[str, int] suggests a tuple of two items, the first a string and the second an integer
  • dict[str, str | float] suggests a dictionary with all keys as strings and values either a string or a float
• Imports that are only used for type hinting can be wrapped in an if TYPE_CHECKING: block to prevent circular imports.

Type hint review process

• During a pull request review, if type hints are missing, the reviewer should add a comment to one line without type hints with a suggested code block on proper type hinting. The lack of type hints should not hold up a pull request that is otherwise ready to be merged.

[krzywon]

This will be sent out for review after the new year.

[pkienzle]

Add guidance for typing of numpy ArrayLike inputs and array outputs.

[krzywon]

Add guidance for typing of numpy ArrayLike inputs and array outputs.

This brings up a good point - we will need to ensure we define typing for all external dependencies. I can think of a few other dependencies with objects passed between methods, including lxml and h5py in sasdata. Others I'm wondering about include PySide6, matplotlib, and scipy, but I don't think there are many instances of these, and there is probably a better way to acheive the same goal without passing these types. Perhaps, in the Type Hint Structure heading, we add a bullet below the list/tuple/dictionary definition specific to external dependencies.

I'll take a look at other types we may need to define and add a recommendation here once I have a (semi-)complete list.

[krzywon]

Object types passed that I've found so far include logging.LEVEL, pathlib.Path and pathlib.PurePath, os.PathLike, from importlib.metadata, PackagePath and Distribution, various internal types (sasdata.datainfo.Data1D, sas.system.config.schema_elements.SchemaElement, sasmodels.modelinfo.ModelInfo, etc.), lxml.etree.Element and lxml.etree, and h5py.Dataset.

With this many different object types passed, I think a general statement is better than something specific to numpy. My suggestion is we replace the last bullet point, related to TYPE_CHECKING, under the Type hint structure section with something like:

• Object types should use the first of the following annotation methods that is applicable:
  • Objects with their own typing scheme should use these typing elements, e.g. numpy.typing.NDArray instead of numpy.ndarray
  • Objects already imported or defined within the module should directly use the Object as the type, e.g. pathlib.Path
  • Objects not imported into the module, or objects that would cause a circular import exception, should be imported within an if typing.TYPE_CHECKING: block and type hints should have the class name surround with quotation marks, e.g. data: "Data1D"

[krzywon]

Discussion from todays biweekly meeting: Clarify that numpy arrays (or any iterable) should define the types of variables it holds, but keep it suggested, not required.

[krzywon]

This suggestion has been approved with the proposed changes and @krzywon will write up the final ADR.

[krzywon]

This has been finalized in #3877. Closing.