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

Add support for automatic type inference (enabled by default). #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ambrevar
Copy link
Contributor

I think it's a very useful default. But could it break existing libraries using compilers that do slot type checking?
(CCL is one of the few that does.)

@attila-lendvai
Copy link
Contributor

i think this is too much DWIM-ness that easily leads to confusion.

just because i initialize a slot to some value, it doesn't mean that i want to restrict the type of all future values to that of the initform.

@Ambrevar
Copy link
Contributor Author

Sure, but then you can always specify :type t to mean that this slot can be of any type.
Or we can disable type inference by default.

@Ambrevar
Copy link
Contributor Author

Ambrevar commented Feb 2, 2021

Would you be OK with merging this if we disable type inference by default?

@Ambrevar
Copy link
Contributor Author

@attila-lendvai Friendly ping! :)

@attila-lendvai
Copy link
Contributor

your effort is appreciated, but i don't think this feature belongs into a general-use library.

my reasoning:

  • it badly hurts code readability for not much in return. some lisps ignore the slot's :type declarations while others are rather strict about it. developing on one lisp may completely hide errors/warning that only show up as errors when tested on another lisp.
  • it also increases code complexity and i suspect most people will not use this feature, or straight out don't even know that it exists if it's not on by default. complexity has a cost that is most often ignored.
  • whoever decides to specify the slot's type is better off doing it explicitly

is this convincing enough?

@Ambrevar
Copy link
Contributor Author

Ambrevar commented Feb 23, 2021 via email

@Ambrevar
Copy link
Contributor Author

I've updated the patch to disable type inference by default (it's now an opt-in feature).

@aartaka
Copy link

aartaka commented Dec 20, 2022

@Ambrevar, don't forget to propagate atlas-engineer/nyxt@c3778a2 to type inference here, as nil is often used as "nothing" slot value, not having any particular type to it. With this change in, I'd consider this feature quite complete and useful, given that we've been using it in Nyxt for more than a year now to control the types of slots and diagnose faulty code paths.

@attila-lendvai, I'd consider one year in an OO-heavy project a good test period for a feature that's otherwise addressing your concerns (given that type inference is off by default now, there's nothing else to worry about)!

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

Successfully merging this pull request may close these issues.

3 participants