Page MenuHomePhabricator

Maybe create an annotation for "this is strictly a template class"
Closed, WontfixPublic

Description

See D11108 for an example... basically, a method was named incorrectly in a subclass.

From IRC:

<joshuaspence> d'oh
 i figured it out
 i've been using shouldWriteInverseTransaction instead of shouldWriteInverseTransactions
 as the method name
 ManiphestTaskDependsOnTaskEdgeType (the diff that i was copying) seems to have done this as well
<epriestley> ohhhh
 we should fix that
<joshuaspence> sending you a diff
<epriestley> I don't have a great way to detect that in general without doing some kind of "@override" annotation, which seems really heavy to me.
 We might be able to look for similar method names in the visibility-of-extended-methods test, maybe.
<joshuaspence> yeah, i was thinking the same
<-- nickz has quit (Ping timeout: 240 seconds)
<joshuaspence> D11108
<phabot> D11108: Fix method name - https://secure.phabricator.com/D11108
<epriestley> e.g., "class X defines 'shouldWriteInverseTransaction', which is suspiciously similar to overridable method 'shouldWriteInverseTransactions' in parent class Y. If this is not a mistake, annotate the method with "@not-an-override-I-promise""
<joshuaspence> i think @override isn't too bad if it's optional
<epriestley> We could only warn you if you actually used it, though, and I'd guess most code won't.
 Like, it's heavyweight / cumbersome enough (and I encounter these issues so rarely) that I'd probably not bother, realistically.
<joshuaspence> yeah, idk
<epriestley> I wonder if we could annotate base classes as, like "strict template class".
 And then warn on new non-private methods introduced in subclasses.
 Since those are always wrong on EdgeType, PHIDType, Application, and probably a lot of others.
 (They're not always wrong on Controller, DAO, View, etc., so we couldn't always do it.)
<joshuaspence> yeah, that seems sort-of not-so-bad
<epriestley> Not sure if that'd be worth it, but I think it'd be a strong signal for the most copy-pastey classes, at least.
 Which are also probably the classes where an error is most likely to occur.
 Maybe we could do, like:
 - Recognize @override as an option.
 - Allow classes to be annotated as @override-required.
 well
 I don't really want to have to put @override on everything
 I feel like, at least in this codebase, we actually want the opposite.
<joshuaspence> i like the "strict template class" idea best
<epriestley> Like @newapi
 And then you'd mark some classes as "@newapi-required".
 Although "@newapi" is a bad name

Event Timeline

joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added a project: Lint.
joshuaspence added subscribers: joshuaspence, epriestley.
epriestley claimed this task.

At least today, our error rate on this seems so low that it probably isn't worth implementing. We could take another look at it if we see more of it in the future (perhaps after we begin encouraging application development), but we haven't seen more of this that I can recall recently.