-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Useless assigns #6055
Useless assigns #6055
Conversation
@a1 = Array(Int32).new(n) { rand(99_999) } | ||
|
||
random = Random.new(4321) # repeatable random seq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so repeatable huh
@a1 = Array(Int32).new(n) { rand(99_999) } | ||
|
||
random = Random.new(4321) # repeatable random seq | ||
@a2 = Array(Int32).new(n) { rand(99_999) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this code instead be fixed to do what it was intended to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should, but not as a part of this PR.
scripts/generate_unicode_data.cr
Outdated
|
||
upcase_ranges = case_ranges entries, &.upcase | ||
upcase_ranges.select! { |r| r.delta != -1 } | ||
|
||
alternate_ranges = alternate_ranges(downcase_one_ranges) | ||
alternate_ranges(downcase_one_ranges) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any side-effects in alternate_ranges
...
scripts/generate_unicode_data.cr
Outdated
|
||
casefold_ranges = case_ranges entries, &.casefold | ||
case_ranges entries, &.casefold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@veelenga So cool, man! 🎉 Can this be somehow incorporated in the compiler? There are many bugs in the std because of this. For example, can the tool detect unused method arguments? It would be nice to detect this because they are sometimes not forwarded, incorrectly. I wonder how it works in this case: macro macro_call
puts x
end
def foo
x = 1
macro_call
end
foo Will it say that |
@asterite thanks, so cool to hear this from you 😄
Only if we copy and improve some code. I believe the code is far from ideal and I am not a language developer so may have done some errors in the design. But, well, of course, it is possible :) Making the compiler to be dependent on the tool does not make sense IMO. The tool is designed to catch style-inconsistences, so it reports a lot of stuff on Crystal repo (for example, "use
This is a next step. Really easy to do now.
Yes, it will. I even didn't know (expect) such a scenario. But it should be possible to handle it properly, will try. |
Failing to compile because of an unused variable is wrong, and I don't really want any compiler warnings, so I can't see a way to put this in the compiler proper. Were you thinking a |
I was thinking about warnings, and being able to silence them. Similar to Elixir. It's really useful (except that in Elixir they also can warn about false positives, which then makes it really useless and tedious, and that's why I would like to avoid any false positives). |
spec/spec_helper.cr
Outdated
@@ -68,7 +68,7 @@ end | |||
def assert_normalize(from, to, flags = nil) | |||
program = Program.new | |||
program.flags = flags if flags | |||
normalizer = Normalizer.new(program) | |||
# normalizer = Normalizer.new(program) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this not removed?
@RX14 do you still expect me to change something here? |
It's difficult to tell which changes here are justified and which changes are just masking other bugs. The unicode tables generator changes especially are ringing massive alarm bells for me. |
scripts/generate_unicode_data.cr
Outdated
|
||
upcase_ranges = case_ranges entries, &.upcase | ||
upcase_ranges.select! { |r| r.delta != -1 } | ||
|
||
alternate_ranges = alternate_ranges(downcase_one_ranges) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in the ECR template. That's why the tool doesn't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I knew I was missing something there...
b75a895
to
265f6f0
Compare
@@ -87,7 +87,7 @@ class Crystal::Call | |||
# (and we checked that all args are covered), and we | |||
# remove our named args. | |||
sorted_named_args.sort_by! &.[0] | |||
sorted_named_args.each do |(index, named_arg)| | |||
sorted_named_args.each do |(_, named_arg)| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At lines like this it could just stay sorted_named_args.each do |(index, named_arg)|
because I think it's better to have a proper name for it instead of replacing it by an underscore. Even if it's not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention (a la ruby) is typically either _
or _foo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elixir follows the same convention which helps to reduce noise IMO.
@@ -28,7 +28,7 @@ describe "concurrent" do | |||
|
|||
it "re-raises errors from Fibers as ConcurrentExecutionException" do | |||
exception = expect_raises(ConcurrentExecutionException) do | |||
a, b = parallel(raising_job, raising_job) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not useless per-se. It does matter that the return type is a tuple. But it is tested in the following spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this spec is not good then. parallel(raising_job, raising_job)
raises an error and never assigns to a, b. If you are talking about compile-time checking, check out one test below.
@@ -4,7 +4,6 @@ module Crystal | |||
def self.check_type_allowed_in_generics(node, type, msg) | |||
return if type.allowed_in_generics? | |||
|
|||
type = type.union_types.find { |t| !t.allowed_in_generics? } if type.is_a?(UnionType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather check through commit history about this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did. And from this commit bf284d9 i didn't get why we need to do this useless assignment. Ok, will revert it.
@@ -396,7 +396,7 @@ class Crystal::Call | |||
end | |||
|
|||
def def_full_name(owner, a_def, arg_types = nil) | |||
Call.def_full_name(owner, a_def, arg_types = nil) | |||
Call.def_full_name(owner, a_def, arg_types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code transformation would be to ignore arg_types and leave nil
but it seems weird. It should be narrowed a bit down what is the expected thing to do here / add missing spec.
@@ -35,7 +35,7 @@ struct Crystal::System::FileInfo < ::File::Info | |||
flags = ::File::Flags::None | |||
flags |= ::File::Flags::SetUser if @stat.st_mode.bits_set? LibC::S_ISUID | |||
flags |= ::File::Flags::SetGroup if @stat.st_mode.bits_set? LibC::S_ISGID | |||
flags |= ::File::Flags::Sticky if @stat.st_mode.bits_set? LibC::S_ISVTX | |||
flags | ::File::Flags::Sticky if @stat.st_mode.bits_set? LibC::S_ISVTX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't thing this change is good.
@@ -105,25 +105,25 @@ class Digest::MD5 < Digest::Base | |||
def ff(a, b, c, d, x, s, ac) | |||
a += f(b, c, d) + x + ac.to_u32 | |||
a = rotate_left a, s | |||
a += b | |||
a + b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto. I don't think that doing transformations that work only because the statement is the last in body, hence the result value, is good to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't get it. The returned result of a + b
is just the same as a += b
, but doesn't assign a
once again. Am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is far better ~ it returns the result idiomatically without having to work out what will be returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcardiff it shows intention, and cuts out the 'what will happen to a' ~ which is idiomatic Crystal as far as I understand.
@@ -155,7 +155,6 @@ module Float::Printer::CachedPowers | |||
# around *exp* (boundaries included). | |||
def self.get_cached_power_for_binary_exponent(exp) : {DiyFP, Int32} | |||
min_exp = MIN_TARGET_EXP - (exp + DiyFP::SIGNIFICAND_SIZE) | |||
max_exp = MAX_TARGET_EXP - (exp + DiyFP::SIGNIFICAND_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some review to check if there isn't somethings missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great for some code review this technique. I pointed some cases were I don't feel comfortable with the changes either because they are abusing that the expression is the last statement or because they are a flag that something might be missing.
I would revert the former ones. And would prefer some further analysis/add specs to approve those changes.
@@ -109,8 +109,6 @@ module Float::Printer::IEEE | |||
physical_significand_is_zero = (d64 & SIGNIFICAND_MASK_64) == 0 | |||
|
|||
lower_bound_closer = physical_significand_is_zero && (exponent(d64) != DENORMAL_EXPONENT_64) | |||
calcualted_exp = exponent(d64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs some review to check if there isn't somethings missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No offense intended, but I feel like this kind of comment is likely to stall a lot of useful PRs because it suggests that there are problems but doesn't move us closer to resolving such problems.
If you suspect there really is a problem but are unsure, will you please tag someone who has experience in that part of the codebase, so we can progress the PR? Otherwise it will just go stale and we'll end up losing useful contributions (and motivation).
end | ||
|
||
def gg(a, b, c, d, x, s, ac) | ||
a += g(b, c, d) + x + ac.to_u32 | ||
a = rotate_left a, s | ||
a += b | ||
a + b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
end | ||
|
||
def hh(a, b, c, d, x, s, ac) | ||
a += h(b, c, d) + x + ac.to_u32 | ||
a = rotate_left a, s | ||
a += b | ||
a + b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
end | ||
|
||
def ii(a, b, c, d, x, s, ac) | ||
a += i(b, c, d) + x + ac.to_u32 | ||
a = rotate_left a, s | ||
a += b | ||
a + b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Thanks everyone for the collaboration. I don't plan to continue working on this PR. |
Merging this PR would be IMO beneficial. |
This is disappointing. I would really encourage PR participants to provide more constructive feedback rather than get bogged down in minor issues before vanishing from the discussion. If we get in the habit of reviewing pull requests like that, the original contributor is more likely to feel discouraged and give up. |
@DestyNova We are just following the rule "If it's not broken, don't change it". Merging this PR might provide a slight readability benefit, but it can silently break some code, and then we'll have more work to do. We already found a few cases where the tool didn't work well, and having to inspect case by case is a lot of work. |
A solution to mitigate this "silent breaks" on main releases, that will impact everyone, is to have a pre-release/beta channel. I know Crystal hasn't reached yet the 1.0 milestone, however the user number starts to be quite big. This can be discussed in a dedicated issue. |
@j8r we have nightlies they're just impossible to use because they're not published to an APT repo or at a consistent URL. |
Can publish pre-releases be a solution @RX14? Something like |
nah, nightlies are fine, it just needs to be made far easier to actually run and test libraries against nightly crystal builds |
This PR is about to remove some of the useless assigns on Crystal repo detected automatically using static code analysis (see crystal-ameba/ameba#41).