abstract method decls; explicit_method_decls provisional feature
#386
Conversation
|
PR Verification: ✅ success |
py/dml/dmlparse.py
Outdated
| def maybe_colon_yes(t): | ||
| '''maybe_colon : COLON''' | ||
| if not site(t).provisional_enabled(provisional.explicit_method_decls): | ||
| report(ESYNTAX(site(t), ':', "expected '=' or 'default'")) |
| assert abstract_decls | ||
| for decl in abstract_decls: | ||
| # We favor reporting EABSTEMPLATE (done by the caller) | ||
| # over EABSMETH | ||
| if not decl.shared: | ||
| report(EABSMETH(decl.site, name)) |
There was a problem hiding this comment.
It would maybe be cleaner if checking abstract methods+params was a single responsibility? The overlap between EABSMETH and EABSTEMPLATE feels somewhat unnatural, maybe it would suffice with EABSMETH and ENPARAM, and give both an optional template arg? or even a mandatory template arg, that may be a file?
In any case: no need to fix within this PR.
py/dml/traits.py
Outdated
| @property | ||
| def signature(self): | ||
| '''To be used with e.g. typecheck_method_override''' |
There was a problem hiding this comment.
Maybe a word to describe its intended operations (e.g., is it a valid dict key? what does == between two signatures imply?)
There was a problem hiding this comment.
Honestly, it only exists to simplify calling traits.typecheck_method_override. Perhaps I should remove "e.g.".
Or do you perhaps protest against its existence if that's its only reason to exist? Keep in mind, this is for MethodHandle, which is a very niche class to begin with (only used in the context of process_method_declarations/sort_method_implementations)
py/dml/traits.py
Outdated
| for impl in implementations: | ||
| if impl.rank in rank_to_method: | ||
| for impl in declarations: | ||
| [existing_impl, existing_abstracts] = existing = \ |
There was a problem hiding this comment.
IIRC, style prescribes ( instead of \ when possible to escape newlines
There was a problem hiding this comment.
Well, = is more touchy about what you can replace with parenthesis, but this case works out.
py/dml/traits.py
Outdated
| 'default ' * impl.overridable)) | ||
| elif not existing: | ||
| report(ENOVERRIDEMETH(impl.site, impl.name, | ||
| 'default '*impl.overridable)) |
There was a problem hiding this comment.
space around * (and as always I dislike bool mult but don't insist on it)
| decl_ancestry_map = {} | ||
| for (r, anc_ranks) in minimal_ancestry.items(): | ||
| anc_decls = [anc_decl | ||
| for anc in anc_ranks | ||
| for anc_decl in flattened(rank_to_method[anc])] | ||
|
|
||
| decls = flattened(rank_to_method[r]) if r is not None else (None,) | ||
| for decl in decls: | ||
| decl_ancestry_map[decl] = anc_decls | ||
|
|
||
| method_map_ranks = {} | ||
|
|
||
| def calc_highest_impls(r): | ||
| existing = method_map_ranks.get(r) | ||
| if existing is not None: | ||
| return | ||
|
|
||
| anc_impl_ranks = Set() | ||
| some_abstract = False | ||
| for anc in minimal_ancestry[r]: | ||
| [anc_impl, anc_abstracts] = rank_to_method[anc] | ||
| if anc_impl is not None: | ||
| anc_impl_ranks.add(anc) | ||
| else: | ||
| some_abstract = True | ||
| calc_highest_impls(anc) | ||
| anc_impl_ranks.update(method_map_ranks[anc]) | ||
|
|
||
| if some_abstract: | ||
| anc_impl_ranks = get_highest_ranks(anc_impl_ranks) | ||
| method_map_ranks[r] = anc_impl_ranks | ||
|
|
||
| calc_highest_impls(None) | ||
| for (r, (impl, abstracts)) in rank_to_method.items(): | ||
| if (impl is not None and not impl.shared | ||
| and impl.site.provisional_enabled( | ||
| provisional.explicit_method_decls)): | ||
| existing = abstracts or decl_ancestry_map[impl] | ||
| if impl.explicit_decl: | ||
| if existing: | ||
| report(EOVERRIDEMETH(impl.site, existing[0].site, | ||
| impl.name, | ||
| 'default ' * impl.overridable)) | ||
| elif not existing: |
There was a problem hiding this comment.
If I understand correctly, you are mutating three dicts in multiple phases. I find it somewhat hard to follow. Can it be improved somehow? e.g., if mutation spaghetti is necessary, then encapsulate it further into a function. Alternatively, if it's not too expensive to create one of the tables at a time, then that could also be an option.
Also, on the surface this looks like a bit more code than before to execute per method, so we might want a quick check that it doesn't visibly degrade performance.
lwaern-intel
force-pushed
the
lw/explicit_method_decls
branch
from
dc3cf35 to
9856a4f
Compare
3 participants
I'll split up the commit into two (one for
explicit_method_decls; one for abstract method decls) once this is ready to merge