Useless assigns #6055

veelenga · 2018-05-03T16:04:12Z

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).

oprypin · 2018-05-03T16:11:56Z

samples/red_black_tree.cr

    @a1 = Array(Int32).new(n) { rand(99_999) }
-
-    random = Random.new(4321) # repeatable random seq


Not so repeatable huh

yxhuvud · 2018-05-03T16:14:07Z

samples/red_black_tree.cr

    @a1 = Array(Int32).new(n) { rand(99_999) }
-
-    random = Random.new(4321) # repeatable random seq
    @a2 = Array(Int32).new(n) { rand(99_999) }


Shouldn't this code instead be fixed to do what it was intended to do?

It should, but not as a part of this PR.

RX14 · 2018-05-03T16:48:01Z

scripts/generate_unicode_data.cr


 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)


I can't see any side-effects in alternate_ranges...

RX14 · 2018-05-03T16:48:26Z

scripts/generate_unicode_data.cr


-casefold_ranges = case_ranges entries, &.casefold
+case_ranges entries, &.casefold


asterite · 2018-05-03T17:37:05Z

@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 x is unused? If so, the tool might not be very good, because it can give false positives. In that case, maybe we should remove the ability to reference variables like that inside macros.

veelenga · 2018-05-03T17:51:19Z

@asterite thanks, so cool to hear this from you 😄

Can this be somehow incorporated in the compiler?

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 method_name instead of methodName")

For example, can the tool detect unused method arguments?

This is a next step. Really easy to do now.

Will it say that x is unused?

Yes, it will. I even didn't know (expect) such a scenario. But it should be possible to handle it properly, will try.

RX14 · 2018-05-03T18:11:30Z

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 crystal tool unused?

asterite · 2018-05-03T18:38:04Z

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).

straight-shoota · 2018-05-03T21:45:11Z

spec/spec_helper.cr

@@ -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)


why is this not removed?

veelenga · 2018-05-08T12:22:40Z

@RX14 do you still expect me to change something here?

RX14 · 2018-05-08T20:09:05Z

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.

asterite · 2018-05-08T20:13:43Z

scripts/generate_unicode_data.cr


 upcase_ranges = case_ranges entries, &.upcase
 upcase_ranges.select! { |r| r.delta != -1 }

-alternate_ranges = alternate_ranges(downcase_one_ranges)


This is used in the ECR template. That's why the tool doesn't see it.

Thanks! I knew I was missing something there...

crystal-lang/crystal#6055 (comment)

wooster0 · 2018-06-05T13:03:06Z

src/compiler/crystal/semantic/lib.cr

@@ -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)|


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.

The convention (a la ruby) is typically either _ or _foo

Elixir follows the same convention which helps to reduce noise IMO.

bcardiff · 2018-06-06T08:21:48Z

spec/std/concurrent_spec.cr

@@ -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)


This is not useless per-se. It does matter that the return type is a tuple. But it is tested in the following spec.

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.

bcardiff · 2018-06-06T12:10:48Z

src/compiler/crystal/semantic/ast.cr

@@ -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)


I would rather check through commit history about this change

I did. And from this commit bf284d9 i didn't get why we need to do this useless assignment. Ok, will revert it.

bcardiff · 2018-06-06T12:13:03Z

src/compiler/crystal/semantic/call_error.cr

@@ -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)


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.

bcardiff · 2018-06-06T12:20:00Z

src/crystal/system/unix/file_info.cr

@@ -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


I don't thing this change is good.

bcardiff · 2018-06-06T12:21:48Z

src/digest/md5.cr

@@ -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


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.

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?

Actually this is far better ~ it returns the result idiomatically without having to work out what will be returned

@bcardiff it shows intention, and cuts out the 'what will happen to a' ~ which is idiomatic Crystal as far as I understand.

bcardiff · 2018-06-06T12:22:57Z

src/float/printer/cached_powers.cr

@@ -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)


This needs some review to check if there isn't somethings missing

bcardiff

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.

bcardiff · 2018-06-06T12:23:01Z

src/float/printer/ieee.cr

@@ -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)


This needs some review to check if there isn't somethings missing

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).

mjago · 2018-06-13T22:24:37Z

src/digest/md5.cr

  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


mjago · 2018-06-13T22:24:52Z

src/digest/md5.cr

  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


mjago · 2018-06-13T22:25:10Z

src/digest/md5.cr

  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


veelenga · 2018-07-05T13:32:21Z

Thanks everyone for the collaboration. I don't plan to continue working on this PR.

Sija · 2018-09-01T13:54:44Z

Merging this PR would be IMO beneficial.

DestyNova · 2018-09-01T14:01:03Z

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.

asterite · 2018-09-01T14:28:55Z

@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.

j8r · 2018-09-01T15:19:23Z

A solution to mitigate this "silent breaks" on main releases, that will impact everyone, is to have a pre-release/beta channel.
A limited group of users can test their code against before a major release, this can prevent releasing a patch version few weeks after like the 0.25.1 and 0.26.1.

I know Crystal hasn't reached yet the 1.0 milestone, however the user number starts to be quite big.
I think this can help everyone to be more confident, and both shards and Crystal to be a part of a more stable ecosystem.

This can be discussed in a dedicated issue.

RX14 · 2018-09-01T16:00:30Z

@j8r we have nightlies they're just impossible to use because they're not published to an APT repo or at a consistent URL.

j8r · 2018-09-01T16:19:02Z

Can publish pre-releases be a solution @RX14? Something like 0.27.0-rc1. I've seen one in the past for the 0.24.0 (maybe an experiment).

RX14 · 2018-09-01T16:39:48Z

nah, nightlies are fine, it just needs to be made far easier to actually run and test libraries against nightly crystal builds

asterite · 2018-09-01T16:46:24Z

@j8r @RX14 Yesterday @bcardiff told me that he's working on getting nightlies to be easy to install (so distributed as packages, I think) so this might be possible soon. I can't remember the exact words he told me, so just don't expect exactly what I'm saying, and also be patient :-)

veelenga added 3 commits May 3, 2018 12:55

Remove useless assignments in spec

f61a137

Remove useless assignments in samples and scripts

d955b02

Remove useless assignments in src

ebd3918

oprypin reviewed May 3, 2018

View reviewed changes

yxhuvud reviewed May 3, 2018

View reviewed changes

RX14 requested changes May 3, 2018

View reviewed changes

Remove method calls without side effects

2dd1be9

wooster0 approved these changes May 3, 2018

View reviewed changes

straight-shoota reviewed May 3, 2018

View reviewed changes

Remove commented lines

5e88918

sdogruyol approved these changes May 7, 2018

View reviewed changes

asterite requested changes May 8, 2018

View reviewed changes

Revert assigns used in ECR templates

265f6f0

veelenga force-pushed the fix/useless-assigns branch from b75a895 to 265f6f0 Compare May 8, 2018 20:28

veelenga added a commit to crystal-ameba/ameba that referenced this pull request May 12, 2018

Prevent false positiveness cause by macro literals

4154327

crystal-lang/crystal#6055 (comment)

wooster0 reviewed Jun 5, 2018

View reviewed changes

bcardiff reviewed Jun 6, 2018

View reviewed changes

bcardiff requested changes Jun 6, 2018

View reviewed changes

mjago reviewed Jun 13, 2018

View reviewed changes

src/digest/md5.cr

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

Copy link

Contributor

mjago Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

mjago reviewed Jun 13, 2018

View reviewed changes

src/digest/md5.cr

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

Copy link

Contributor

mjago Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

mjago reviewed Jun 13, 2018

View reviewed changes

src/digest/md5.cr

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

Copy link

Contributor

mjago Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

faustinoaq mentioned this pull request Jun 24, 2018

Add ameba amberframework/granite#240

Merged

veelenga closed this Jul 5, 2018

Sija mentioned this pull request Jan 9, 2021

Fix selected issues found by Ameba #10230

Closed

HertzDevil mentioned this pull request Jan 5, 2022

Remove useless variable declarations in trailing position #11704

Merged

		@a1 = Array(Int32).new(n) { rand(99_999) }

		random = Random.new(4321) # repeatable random seq


		casefold_ranges = case_ranges entries, &.casefold
		case_ranges entries, &.casefold

Useless assigns #6055

Useless assigns #6055

Conversation

veelenga commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented May 3, 2018

veelenga commented May 3, 2018 • edited

RX14 commented May 3, 2018

asterite commented May 3, 2018

Choose a reason for hiding this comment

veelenga commented May 8, 2018

RX14 commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooster0 Jun 5, 2018 • edited

Choose a reason for hiding this comment

z64 Jun 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga commented Jul 5, 2018 • edited

Sija commented Sep 1, 2018

DestyNova commented Sep 1, 2018

asterite commented Sep 1, 2018

j8r commented Sep 1, 2018

RX14 commented Sep 1, 2018

j8r commented Sep 1, 2018

RX14 commented Sep 1, 2018 • edited

asterite commented Sep 1, 2018

veelenga commented May 3, 2018 •

edited

wooster0 Jun 5, 2018 •

edited

z64 Jun 5, 2018 •

edited

veelenga Jun 6, 2018 •

edited

veelenga Jun 6, 2018 •

edited

veelenga Jun 6, 2018 •

edited

veelenga commented Jul 5, 2018 •

edited

RX14 commented Sep 1, 2018 •

edited