Change Details

With very rare exceptions, classes in a phutil/Phabricator codebase should be `abstract` or `final` (probably, we should make this rule "always" instead of "with very rare exceptions", since I think all the exception cases turned out to be bad ideas and we no longer introduce them). Ancient history on this is approximately T795, but my recollection is: - Sometimes users took the absence of `final` to mean "I can extend this". - T795 was originally going to mark things which could be extended as `@stable` but not use `final`, but we ended up just `final`-ing everything instead. - Removing `final` is a clear "void the warranty" step that makes it obvious to users that they're in "fork" territory, not "the upstream is happy to support you" territory. - Earlier we had some sub-rules around this (`@stable`, `@concrete-extensible`) but as the codebase evolved these became effectively unnecessary. The modern "modular" pattern (abstract base class, final subclasses, use runtime introspection to load them with `PhutilClassMapQuery`) hit its stride after these changes and proved much better than all the stuff that came before it, so we just use that everywhere now. We could eventually remove any vestigial traces of `@stable` or `@concrete-extensible`. The existing `ArcanistRaggedClassTreeEdgeXHPASTLinterRule` in `arcanist/` enforces these rules, but it is disabled by default. libphutil-specific rules like this are normally disabled by default so that the XHPAST linter's default settings are more reasonable for PHP in general. The `ArcanistPhutilXHPASTLinterStandard` (`"phutil.xhpast"`) ruleset reconfigures the linter's default settings to be appropriate for libphutil development, but doesn't enable this rule. To fix this: - Add this rule to `ArcanistPhutilXHPASTLinterStandard->getLinterSeverityMap()`, probably as a "WARNING" (we could make it an error after getting rid of `@concrete-extensible` completely, maybe) in `arcanist/`. To test: - Create a new change in Phabricator or libphutil which adds a `class` that is not`abstract` or `final`. - Run `arc lint`. The rule itself already has a unit test and I'm not sure trying to integration test the default behavior of `arc lint` is necessarily too useful (lots of work, issues like this are very rare) so I don't think we need any expansion of test coverage.

With very rare exceptions, classes in a phutil/Phabricator codebase should be `abstract` or `final` (probably, we should make this rule "always" instead of "with very rare exceptions", since I think all the exception cases turned out to be bad ideas and we no longer introduce them). Ancient history on this is approximately T795, but my recollection is: - Sometimes users took the absence of `final` to mean "I can extend this". - T795 was originally going to mark things which could be extended as `@stable` but not use `final`, but we ended up just `final`-ing everything instead. - Removing `final` is a clear "void the warranty" step that makes it obvious to users that they're in "fork" territory, not "the upstream is happy to support you" territory. - Earlier we had some sub-rules around this (`@stable`, `@concrete-extensible`) but as the codebase evolved these became effectively unnecessary. The modern "modular" pattern (abstract base class, final subclasses, use runtime introspection to load them with `PhutilClassMapQuery`) hit its stride after these changes and proved much better than all the stuff that came before it, so we just use that everywhere now. We could eventually remove any vestigial traces of `@stable` or `@concrete-extensible`. The existing `ArcanistRaggedClassTreeEdgeXHPASTLinterRule` in `arcanist/` enforces these rules, but it is disabled by default. libphutil-specific rules like this are normally disabled by default so that the XHPAST linter's default settings are more reasonable for PHP in general. The `ArcanistPhutilXHPASTLinterStandard` (`"phutil.xhpast"`) ruleset reconfigures the linter's default settings to be appropriate for libphutil development, but doesn't enable this rule. To fix this: - Add this rule to `ArcanistPhutilXHPASTLinterStandard->getLinterSeverityMap()`, probably as a "WARNING" (we could make it an error after getting rid of `@concrete-extensible` completely, maybe) in `arcanist/`. To test: - Create a new change in Phabricator or libphutil which adds a `class` that is not `abstract` or `final`. - Run `arc lint`. The rule itself already has a unit test and I'm not sure trying to integration test the default behavior of `arc lint` is necessarily too useful (lots of work, issues like this are very rare) so I don't think we need any expansion of test coverage.

With very rare exceptions, classes in a phutil/Phabricator codebase should be `abstract` or `final` (probably, we should make this rule "always" instead of "with very rare exceptions", since I think all the exception cases turned out to be bad ideas and we no longer introduce them). Ancient history on this is approximately T795, but my recollection is: - Sometimes users took the absence of `final` to mean "I can extend this". - T795 was originally going to mark things which could be extended as `@stable` but not use `final`, but we ended up just `final`-ing everything instead. - Removing `final` is a clear "void the warranty" step that makes it obvious to users that they're in "fork" territory, not "the upstream is happy to support you" territory. - Earlier we had some sub-rules around this (`@stable`, `@concrete-extensible`) but as the codebase evolved these became effectively unnecessary. The modern "modular" pattern (abstract base class, final subclasses, use runtime introspection to load them with `PhutilClassMapQuery`) hit its stride after these changes and proved much better than all the stuff that came before it, so we just use that everywhere now. We could eventually remove any vestigial traces of `@stable` or `@concrete-extensible`. The existing `ArcanistRaggedClassTreeEdgeXHPASTLinterRule` in `arcanist/` enforces these rules, but it is disabled by default. libphutil-specific rules like this are normally disabled by default so that the XHPAST linter's default settings are more reasonable for PHP in general. The `ArcanistPhutilXHPASTLinterStandard` (`"phutil.xhpast"`) ruleset reconfigures the linter's default settings to be appropriate for libphutil development, but doesn't enable this rule. To fix this: - Add this rule to `ArcanistPhutilXHPASTLinterStandard->getLinterSeverityMap()`, probably as a "WARNING" (we could make it an error after getting rid of `@concrete-extensible` completely, maybe) in `arcanist/`. To test: - Create a new change in Phabricator or libphutil which adds a `class` that is not `abstract` or `final`. - Run `arc lint`. The rule itself already has a unit test and I'm not sure trying to integration test the default behavior of `arc lint` is necessarily too useful (lots of work, issues like this are very rare) so I don't think we need any expansion of test coverage.