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

Fix all 'assigned but unused variable' warnings #1551

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

skipkayhil
Copy link
Contributor

  • The variables in content_location_parser appear to have been copied from content_disposition_parser but are unused
  • Ragel seems to always generate a useless testEof variable. The if false trick is also used in whitequark/parser@21581a2 to suppress the warning. As mentioned in the parser commit, if false will not actually generate instructions and so it has no runtime cost.

The first commit is simply running rake ragel again to regenerate the parsers

Fixes #1400 (I've added @GQuirino as a Co-Author)

skipkayhil and others added 2 commits December 23, 2022 15:21
The formatting differences are likely due to using a newer version of
rufo
- The variables in content_location_parser appear to have been copied
  from content_disposition_parser but are unused
- Ragel seems to always generate a useless testEof variable. The `if
  false` trick is also used in whitequark/parser@21581a2 to suppress the
  warning. As mentioned in the parser commit, `if false` will not
  actually generate instructions and so it has no runtime cost.

Co-authored-by: Guilherme Quirino <guilhermequirino74@gmail.com>
@skipkayhil
Copy link
Contributor Author

skipkayhil commented Dec 23, 2022

I tried to wrap my head around the statement not reached warnings but haven't cracked it.

An example is this:

# lib/mail/parsers/content_location_parser.rb
when 17
  # line 24 "lib/mail/parsers/rfc5322_lexical_tokens.rl"
  begin
    begin
      stack[top] = cs
      top += 1
      cs = 20
      _goto_level = _again
      next
    end
  end
  # line 20 "lib/mail/parsers/content_location_parser.rl"
  begin
    content_location.location = chars(data, token_string_s, p - 1)
  end

The comments indicate that the first block is generated by comment_begin, and the second block by token_string_e.

Mostly just dumping some thoughts, I think this PR should be merged whether or not the other warnings are addressed.

@sebbASF
Copy link
Collaborator

sebbASF commented Dec 24, 2022

I wondered about that too at first.

I think the warning is caused by the use of an unconditional 'next' which presumably terminates the 'when' block, so line 20 is not reached.

@sebbASF
Copy link
Collaborator

sebbASF commented Dec 24, 2022

I wonder if it is worth fixing the warnings, given that the files are automatically generated.

It might be better to set things up so warnings are not generated for these files.

skipkayhil added a commit to skipkayhil/rails that referenced this pull request Feb 23, 2023
Previously, requiring any of the ragel generated parsers in mail would
output tons of warnings in tests, making output much harder to read
(especially in Railties).

This commit extends the RaiseWarnings patch to suppress output from
these mail parsers.

The suppression can be removed once mikel/mail#1557 or mikel/mail#1551
or any other PR fixes the issue
@skipkayhil
Copy link
Contributor Author

It might be better to set things up so warnings are not generated for these files.

Since the parser gem uses the same trick to fix the testEof warning, I would be surprised if there was a better way. The rest of the warnings are actually unused variables, so removing them fixes the warnings.

I opened #1557 as another potential method to silence warnings, but someone pointed out to me that changing a global variable in that way is not thread safe.

Whether or not we find a solution to unreachable statement warnings, I think this change is worth merging as it is an improvement on the current situation.

Copy link
Collaborator

@sebbASF sebbASF left a comment

Choose a reason for hiding this comment

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

Looks OK to me

@eval
Copy link
Collaborator

eval commented Jun 3, 2024

Thanks!

@skipkayhil skipkayhil deleted the fix-warnings branch June 9, 2024 04:43
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