Add rules for enforcing type restrictions on vars and parameters

[nobodywasishere]

I'm imagining this would be a few rules, and not sure what namespace they would fall under. Something like:

Typing/InstanceVarTypeRestriction: # and ClassVarTypeRestriction
  Enabled: true

Typing/LocalVarTypeRestriction:
  Enabled: true
  
Typing/MethodParamTypeRestriction:
  Enabled: true
  DefaultValue: false
  PrivateMethods: false # don't require on private methods
  ProtectedMethods: false
  BlockParam: false # whether typing is required for block params

Typing/MethodReturnTypeRestriction:
  Enabled: true
  PrivateMethods: false
  ProtectedMethods: false

Typing/ProcReturnTypeRestriction:
  Enabled: true

# Maybe some others, can't think of any others currently

This would be disabled by default. It wouldn't be perfect, but would definitely be good enough for my use-cases. If this feature would be welcome, I can start prototyping an implementation.

[nobodywasishere]

For getter/setter/record, there could be a rule like:

Typing/MacroCallVarTypeRestriction:
  Enabled: true
  DefaultValue: true
  MacroNames:
   - record
   - getter
   - setter
   - property

[straight-shoota]

Instance variables always need to be typed. The compiler is able to infer the type in some cases from default values.
So I'm wondering what InstanceVarTypeRestriction with DefaultValue: false would complain about other than what is already a compiler error? Same for ClassVarTypeRestriction.

[nobodywasishere]

@straight-shoota That's valid. Instance/class var ones shouldn't have the DefaultValue option then.

[Sija]

• InstanceVarTypeRestriction and ClassVarTypeRestriction: doesn't IMO make much sense, since ivars/cvars need to have the type, either defined explicitly or inferred
• LocalVarTypeRestriction: I'm against this one, since typing local vars is more a problem than a solution really
• MethodParamTypeRestriction and MethodReturnTypeRestriction: I'm ok with
• ProcReturnTypeRestriction: by proc you meant block?
• MacroCallVarTypeRestriction: could you provide any use-case?

[devnote-dev]

• InstanceVarTypeRestriction and ClassVarTypeRestriction: doesn't IMO make much sense, since ivars/cvars need to have the type, either defined explicitly or inferred

That's all good and well from the compiler's perspective, but isn't the slightest bit helpful to users of, for example, a shard which makes public use of otherwise "untyped" instance variables:

class Foo
  getter bar

  def self.construct
    new 123
  end

  protected def initialize(@bar : Int32)
  end
end

foo = Foo.construct
foo.bar # => 123

• LocalVarTypeRestriction: I'm against this one, since typing local vars is more a problem than a solution really

In what scenarios? I make a continuous effort to explicitly type local variables where it would otherwise be inferred to prevent the compiler from making assumptions. This is especially useful when handling nilable cases like the following, and I'm surprised that it's not a common practice:

some_var = nil # left to be inferred, doesn't help with debugging
some_var : Int32? = nil # ok, now we have a general idea of how this var should be used

While my common use case is for nilable variables, I'm sure there are other cases where it would be just as useful.

• ProcReturnTypeRestriction: by proc you meant block?

foo = -> (x : Int32, y : Int32) : Int32 { x + y; nil }
p foo.call 3, 4 # Error: expected Proc to return Int32, not Nil

[nobodywasishere]

I should preface that part of the purpose of this PR is to enforce a subset of the language in which type inference is limited as much as practical, for style, clarity, and tooling support. I'm not saying everyone should use these rules (except for maybe the method param/return type ones), but for me and others, type inference has some downsides and should be limited or known when it's being done. I'm not advocating for eliminating inference either, as it is useful. But it's more useful during quick prototyping and testing. When it comes to cleaning up code for prod or release, whether that's an app or a library for others to use, having all of the types there makes errors more readable and using the code easier. On top of that, the more inference is limited, the less tooling needs to rely on how a method/class/other thing is used, and can just rely on the types provided. Tooling would be able to provide (some) semantic errors for methods that aren't used.

InstanceVarTypeRestriction/ClassVarTypeRestriction and MacroCallVarTypeRestriction are related, the latter being for macro calls like record or property which define instance vars and methods.

[nobodywasishere]

I have prototype implementations for some of these rules on this branch

[straight-shoota]

I believe it could be an interesting option to limit the effect of the method linters to documented methods, i.e. ones that are part of the public API.

Type restrictions on parameters and return values are very useful as they enhance the documentation by signaling which types a consumer should expect to use. But this is less valuable for an internal helper method that's never intended for external usage. In that case it might be more of a nuisance to full type all parameters and return values.

[Sija]

In what scenarios? I make a continuous effort to explicitly type local variables where it would otherwise be inferred to prevent the compiler from making assumptions. This is especially useful when handling nilable cases like the following, and I'm surprised that it's not a common practice:

@devnote-dev Perhaps that's your personal preference, yet most of the Crystal code I've seen (incl. Crystal itself and many other widely used shards/apps) is not doing that, usually because:

1. There's no practical reason to do so
2. Makes the code looks ugly and un-Crystal-ish

If you have any good use-cases I'm open to hearing to about them.

[nobodywasishere]

@straight-shoota That's a good idea!

[devnote-dev]

1. There's no practical reason to do so
2. Makes the code looks ugly and un-Crystal-ish

@Sija Is type safety impractical? You're right that most Crystal projects don't use it, but not because of the points listed. Many people, especially those new to Crystal, aren't aware that it's something you can do, or they aren't aware of its benefits. What I often find (even for myself) when looking into Crystal projects is that I'm mimicking the compiler's resolution logic for figuring out the type of a variable—that is, tracing all of its callers—whether it be local variable or a method parameter. That's neither intuitive nor productive.

I completely disagree with your second point just because it doesn't make sense. It's the same syntax you use for macros like property and getter. A lack of a caller prefix doesn't make it ugly.

[nobodywasishere]

@Sija Would you be okay with me adding these rules, even if you think they're impractical, because they're important and useful for me and others?

[nobodywasishere]

Here's an example in favor of typing: https://carc.in/#/r/hfvc

Without a type restriction, variables can be reassigned to any other type, intentionally or accidentally, and this can happen anywhere in the code. I have run into this issue several times where I make a mistake and re-use the same var within the same scope and run into weird behavior / missing method errors; or when I don't know the real type of a var/ivar/cvar because the type was left up to inference among the million different places it could be assigned to and all I can do is guess and check, taking a significant amount of time and effort.

Requiring a type restriction is the equivalent of let/var/const in other language, declaring the variable explicitly rather than leaving it to inference. It is taking the mental model that the original programmer had when they wrote the code, and including it in the program itself (instead of leaving it up to future devs to figure it out).

For me, trying to ramp up on non-trivial Ruby or Crystal code where there are basically no types anywhere is impossible without many hours doing what devnote said of going through every call and assignment to figure out what's really going on. It can be really painful, especially when working on a codebase as large as Kagi's.

[nobodywasishere]

I'm not trying to inflame or cause stress or conflict either, and I apologize if I have been doing that, especially with the high number of issues and PRs to Ameba recently. I care a lot about Crystal and really enjoy being a part of this community. My goal is for tooling that helps us avoid mistakes, that helps us write code that's easier to understand and work on, and I believe that this is a step in the right direction.

[Sija]

@Sija Is type safety impractical? You're right that most Crystal projects don't use it, but not because of the points listed. Many people, especially those new to Crystal, aren't aware that it's something you can do, or they aren't aware of its benefits. What I often find (even for myself) when looking into Crystal projects is that I'm mimicking the compiler's resolution logic for figuring out the type of a variable—that is, tracing all of its callers—whether it be local variable or a method parameter. That's neither intuitive nor productive.

@devnote-dev I'm still waiting to hear about the benefits you've mentioned.

I completely disagree with your second point just because it doesn't make sense. It's the same syntax you use for macros like property and getter. A lack of a caller prefix doesn't make it ugly.

Perhaps for you it does not. Crystal appeal for many is its syntax, especially the type-inference part. re: property macros and alike, these in most cases simply require type annotations to make compiler happy, so they're pretty unavoidable, for local variables on the other hand there's no need to use them, ever.

[Sija]

@nobodywasishere Firstly I wanted to Thank You for your consideration and a meritocratic tone of your comments. This really helps in moving the discussion forward.

@Sija Would you be okay with me adding these rules, even if you think they're impractical, because they're important and useful for me and others?

I'd be open to adding all of the rules mentioned in the OP, except the LocalVarTypeRestriction which I still need some convincing about its usefulness.

Thanks again for Your valuable feedback! 🙏

[nobodywasishere]

for local variables on the other hand there's no need to use them, ever.

I agree that you don't need to, and the compiler can figure it out for you. But we're asking for a small rule that would let us know when we are letting the compiler make that assumption, and ways to limit how often that assumption is being made. There are other ways to achieve that though.

I'm going to keep testing local changes I have and create a few PRs for the other typing rules, primarily Typing/MethodParamTypeRestriction, Typing/MethodReturnTypeRestriction, Typing/ProcReturnTypeRestriction, and Typing/MacroCallVarTypeRestriction. Those are implemented and work well, though could use with a few more specs just to be sure.

Typing/LocalVarTypeRestriction is implemented, though there's still discussion to be had on whether it's worth adding. The other two for Instance and Class vars still need some work, the implementation for local vars will probably help with that at the very least.

[nobodywasishere]

I still want to add Typing/InstanceVarTypeRestriction and Typing/ClassVarTypeRestriction, but those will require semantic information to do correctly. If this issue is closed I'll open another issue for those so we don't lose track of them