Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable_partially_abstract_typeconsts hhconfig option can not be set #333

Closed
lexidor opened this issue Dec 18, 2020 · 7 comments
Closed

Comments

@lexidor
Copy link
Contributor

lexidor commented Dec 18, 2020

I am responsible for committing this error in the first place.
This error got past CI because we don't have this option enabled in our .hhconfig.
If this options is maybe going to become the default in some future release, I need to fix this.

Typing[4138] The assigned type of this type constant is inconsistent with its parent [1]
-> This shape type allows unknown fields, and so it may contain fields other than those explicitly declared in its declaration. [2]
-> It is incompatible with a shape that does not allow unknown fields. [3]

src/Linters/ConsistentLineEndingsLinter.hack:18:14
      16 |   extends AutoFixingLineLinter<LineLintError> {
      17 |   // Could become configurable later.
[1,3] 18 |   const type TConfig = shape('line ending' => LineEnding);
      19 | 
      20 |   private function getLineEnding(): LineEnding {

src/Linters/BaseLinter.hack:19:38
      17 | abstract class BaseLinter {
      18 |   <<__Reifiable>>
[2]   19 |   const type TConfig as shape(...) = shape(...);
      20 | 
      21 |   abstract public function getLintErrorsAsync(): Awaitable<vec<LintError>>;
@lexidor
Copy link
Contributor Author

lexidor commented Feb 25, 2021

It looks like it was added in 2019 and not touched since then, so I wouldn't expect it to change any time soon. I don't think it's necessary to preemptively fix HHAST or other projects, we can always do that if/when someone wants to change the default.

@lexidor lexidor changed the title [ Regression ] Typeerror because of ConsistentLineEndingsLinter::TConfig when disable_partially_abstract_typeconsts hhconfig option is enabled disable_partially_abstract_typeconsts hhconfig option can not be set Feb 25, 2021
@jjergus
Copy link
Contributor

jjergus commented Feb 26, 2021

That said, if you want to enable this option in some of your projects but the HHAST dependency is preventing you, I certainly wouldn't be opposed to fixing it here.

@lexidor
Copy link
Contributor Author

lexidor commented Feb 26, 2021

That said, if you want to enable this option in some of your projects but the HHAST dependency is preventing you, I certainly wouldn't be opposed to fixing it here.

If I knew what this setting did, I could make an informed decision. I don't understand the error it creates either. If others want to turn this feature on, please comment below.

@lexidor
Copy link
Contributor Author

lexidor commented Apr 20, 2021

It seems like this area is being explored again.
facebook/hhvm@e72c9ef

facebook-github-bot pushed a commit to facebook/hhvm that referenced this issue Apr 23, 2021
Summary: This is not a real HIP following the usual template/process, but we'd like to share it so we can answer questions like hhvm/hhast#333 (similarly to [gradual modularity](https://github.com/facebook/hhvm/blob/master/hphp/hack/doc/HIPs/gradual_modularity.md) which is also not a real HIP).

Reviewed By: fredemmott

Differential Revision: D27923993

fbshipit-source-id: 2239a36d69d91ebe486eb8bdb7d19ca95a6430b8
@jjergus
Copy link
Contributor

jjergus commented Apr 23, 2021

The current state/plan is now documented here: https://github.com/facebook/hhvm/blob/master/hphp/hack/doc/HIPs/converging_constants.md

@lexidor
Copy link
Contributor Author

lexidor commented Apr 23, 2021

Thanks for publishing the HIP. I have given it a read, but I couldn't take it all in at once. I'll read this again tomorrow. (With a hh_client, hhvm, and pen and paper in hand 😄.)

@lexidor
Copy link
Contributor Author

lexidor commented Jan 12, 2022

This issue has been resolved. The type constant does not have a default value anymore.

@lexidor lexidor closed this as completed Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants