ucb-bar / hammer

Hammer: Highly Agile Masks Made Effortlessly from RTL
BSD 3-Clause "New" or "Revised" License
255 stars 59 forks source link

Type-safe get_setting #304

Open edwardcwang opened 5 years ago

edwardcwang commented 5 years ago

Right now get_setting just returns Any which is a type-safety hazard (e.g. with Optional[str] getting passed in as "null"). Would be good to have type-safe versions of get_setting that have a bit more knowledge of the expected return type.

harrisonliew commented 5 years ago

@edwardcwang I see multiple ways of addressing this, and am not sure what your intention was.

A get_setting calls the method in hammer_tool: https://github.com/ucb-bar/hammer/blob/a627b888f5aa6957a3b73e90981881340704b5e9/src/hammer-vlsi/hammer_vlsi/hammer_tool.py#L698-L711

which then calls the method in config_src: https://github.com/ucb-bar/hammer/blob/d7b29a58009ecbe6ca3bb1d87bfd318998cdb838/src/hammer_config/config_src.py#L555-L567

So if get_setting in config_src correctly passes a null value as None, it will return "null" to hammer_tool if nullvalue isn't specified as not None there.

So, do you want hammer_tool's nullvalue to override that in config_src or just make all nullvalues Optional[str] = None?

edwardcwang commented 5 years ago

The issue is that get_setting's static return type can't change depending on its parameters. The solution that I was thinking of was type-specific/safe versions of get_settings. The more sustainable solution is for get_setting to not be used by users directly, though...

jwright6323 commented 5 years ago

You are proposing something like get_setting_str, get_setting_int, get_setting_float, ... ?

edwardcwang commented 5 years ago

In the current Python type-checking regime I don't know of another way to do this.

(On a side note: some of the more fancy programming languages have this concept of "dependent types" which appear to allow the type checker to determine the return type of a function/class based on parameters. With dependent types one is able to side-step this problem)

edwardcwang commented 5 years ago

To add a concrete example we ran into recently: pitch=self.get_setting("vlsi.inputs.bumps.pitch"), gives the wrong/unconstrained default type even though the YAML defines this as a Decimal

jwright6323 commented 5 years ago

To add a concrete example we ran into recently: pitch=self.get_setting("vlsi.inputs.bumps.pitch"), gives the wrong/unconstrained default type even though the YAML defines this as a Decimal

Fortunately @harrisonliew is fixing this particular issue in #372, but we still need a bigger solution to the issue tracked here.

Edit: now fixed in #433