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

Turn abstract return type erros into warnings #7999

Merged
merged 1 commit into from Jul 27, 2019

Conversation

@asterite
Copy link
Member

commented Jul 26, 2019

Follow up to #7956

We agreed that making them errors will cause a massive breaking change.
Making them warnings first will make fixing these things easier while things still keep working.

Turn abstract return type erros into warnings
We agreed that making them errors will cause a massive breaking change.
Making them warnings first will make fixing these things easier while
things still keep working.

@asterite asterite added this to the 0.30.0 milestone Jul 26, 2019

@asterite asterite merged commit b023977 into crystal-lang:master Jul 27, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asterite asterite deleted the asterite:warning-abstract-return-type branch Jul 27, 2019

@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

After this PR these warnings are always emitted, despite --warnings=none.
In other places, before appending something to @program.warning_failures a check is done after:

  • checking @program.warnings.all? (or eventually some other flag configuration per type of warning)
  • checking location of the code via ignore_warning_due_to_location or similar.

Without these two checks, the compiler is a bit more pedantic.

I'm checking if we are satisfied with that current state before releasing

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

Oh, I didn't see that... I though you would emit warnings and in that method they would be ignored if they are not turned on. This is probably slightly slower because the warning message is created, but I'm not sure how much of a slowdown that is...

@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

If we would go that route the warning message would need to have a category field and location so that logic is handled in a single place.

Or we can change the api to be .warning(category, location) { "message" } to lazy build the message if category / location matches. But at least in the deprecation message warning, it was better to have logic upper in a separate place for performance (and that was the only case up to now)

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

I see... I guess for now we can just fix the existing warnings that are always shown then. Could you take care of that? All of them are in abstract_def_checker.cr. Otherwise let me know and I'll do it, but much later today.

@RX14

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

The warning system is only used here for 1 release before this warning is removed and made an error, and warnings (iirc) are still off by default because I was removing the warning system in favour of a simpler system. I was working on this but I'm still struggling to even keep up with github notifications with the time I have to work on crystal...

Let's just leave this as-is, since the whole warnings system may be removed. Discussion in #7655

@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I wasn’t able to spent time on this. I might tomorrow if I finish checking the ecosystem.

I see this as a first time where we can evolve smoothly the language semantics. I find it valuable to build the abstractions to implement these kind of things in the less error prone way as possible.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@bcardiff No worries, I already sent a PR for this :-) (I hope it's fine 🤞)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.