[clang-tidy] Always open files using UTF-8 encoding

[werat]

Summary

The encoding used for opening files depends on the OS and might be different
from UTF-8 (e.g. on Windows it can be CP-1252). The documentation files use
UTF-8 and might be incompatible with other encodings. For example, right now
clang-tools-extra/docs/clang-tidy/checks/abseil-no-internal-dependencies.rst
has non-ASCII quotes and running add_new_check.py fails on Windows, because
it tries to read the file with incompatible encoding.

Use io.open for compatibility with both Python 2 and Python 3.

[kbobyrev]

Hi! Thanks for the patch, it makes sense to me! Honestly, I think having the open shadow is maybe not the best way to solve this (please feel free to let me know if there are reasons it would be better), maybe spell it out explicitly (there aren't that many cases of open)?

[kbobyrev]

For example, right now clang-tools-extra/docs/clang-tidy/checks/abseil-no-internal-dependencies.rst has non-ASCII quotes

Also, hm... Do we really need these? It's safe to remove those, this shouldn't be a problem, I think.

[werat]

Hi! Thanks for the patch, it makes sense to me! Honestly, I think having the open shadow is maybe not the best way to solve this (please feel free to let me know if there are reasons it would be better), maybe spell it out explicitly (there aren't that many cases of open)?

It seemed a bit tedious to spell it out explicitly everywhere, but you're right, not a script of this size it's not a problem. The benefit of a global shadow is that nobody will use regular open accidentally, but I think the risk of that is relatively low.

Also, hm... Do we really need these? It's safe to remove those, this shouldn't be a problem, I think.

I don't think using non-ASCII quotes was intentional and we can definitely remove them. But future-proofing this script will avoid this problem in the future.

[werat]

Use io.open explicitly.

[werat]

Fix incorrect replace

[kbobyrev]

Makes sense, thank you for the explanation. I've changed the problematic doc but this is probably still fine. Honestly, I don't really think we should have more Unicode symbols in the docs for generators hence this patch might not be needed but I'd be OK if you really still want to land it. The "problem" is that we'd be left with somewhat non-uniformity (there are many other Python scripts with open and inputs there might also have similar issues) but I guess that's what we have anyway.

clang-tools-extra/clang-tidy/add_new_check.py

19

The comments look misplaced now. Maybe add it next to the first io.open and end with "Here and elsewhere"?

[werat]

In D106792#2912955, @kbobyrev wrote:

Honestly, I don't really think we should have more Unicode symbols in the docs for generators

I'm not very familiar with the documentation build process, but maybe we could enforce only-ASCII there? Then all scripts and tools can just assume it will work with plan open.

[kbobyrev]

It's certainly doable but looks like a complete overkill for this kind of problem :) The patch is fine, I'm okay with landing it if you're expecting similar problem to arise in the future, I'll accept with the comment next to the first usage, everything else is OK.

[werat]

Moved the comment to the first usage of io.open.

[kbobyrev]

Thanks!

clang-tools-extra/clang-tidy/add_new_check.py

 #!/usr/bin/env python
 #
 #===- add_new_check.py - clang-tidy check generator ---------*- python -*--===#
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===-----------------------------------------------------------------------===#
 
 from __future__ import print_function
 
 import argparse
+import io
 import os
 import re
 import sys
 
-
 # Adapts the module's CMakelist file. Returns 'True' if it could add a new
 # entry and 'False' if the entry already existed.
 def adapt_cmake(module_path, check_name_camel):
   filename = os.path.join(module_path, 'CMakeLists.txt')
-  with open(filename, 'r') as f:
+
+  # The documentation files are encoded using UTF-8, however on Windows the
+  # default encoding might be different (e.g. CP-1252). To make sure UTF-8 is
+  # always used, use `io.open(filename, mode, encoding='utf8')` for reading and
+  # writing files here and elsewhere.
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   cpp_file = check_name_camel + '.cpp'
 
   # Figure out whether this check already exists.
   for line in lines:
     if line.strip() == cpp_file:
       return False
 
   print('Updating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     cpp_found = False
     file_added = False
     for line in lines:
       cpp_line = line.strip().endswith('.cpp')
       if (not file_added) and (cpp_line or cpp_found):
         cpp_found = True
         if (line.strip() > cpp_file) or (not cpp_line):
           f.write('  ' + cpp_file + '\n')
           file_added = True
       f.write(line)
 
   return True
 
 
 # Adds a header for the new check.
 def write_header(module_path, module, namespace, check_name, check_name_camel):
   check_name_dashes = module + '-' + check_name
   filename = os.path.join(module_path, check_name_camel) + '.h'
   print('Creating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     header_guard = ('LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_' + module.upper() + '_'
                     + check_name_camel.upper() + '_H')
     f.write('//===--- ')
     f.write(os.path.basename(filename))
     f.write(' - clang-tidy ')
     f.write('-' * max(0, 42 - len(os.path.basename(filename))))
     f.write('*- C++ -*-===//')
     f.write("""
""" % {'header_guard': header_guard,
        'module': module,
        'namespace': namespace})
 
 
 # Adds the implementation of the new check.
 def write_implementation(module_path, module, namespace, check_name_camel):
   filename = os.path.join(module_path, check_name_camel) + '.cpp'
   print('Creating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     f.write('//===--- ')
     f.write(os.path.basename(filename))
     f.write(' - clang-tidy ')
     f.write('-' * max(0, 51 - len(os.path.basename(filename))))
     f.write('-===//')
     f.write("""
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.

 
 
 # Modifies the module to include the new check.
 def adapt_module(module_path, module, check_name, check_name_camel):
   modulecpp = list(filter(
       lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
       os.listdir(module_path)))[0]
   filename = os.path.join(module_path, modulecpp)
-  with open(filename, 'r') as f:
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   print('Updating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     header_added = False
     header_found = False
     check_added = False
     check_fq_name = module + '-' + check_name
     check_decl = ('    CheckFactories.registerCheck<' + check_name_camel +
                   '>(\n        "' + check_fq_name + '");\n')
 
     lines = iter(lines)
except StopIteration:
       pass
 
 
 # Adds a release notes entry.
 def add_release_notes(module_path, module, check_name):
   check_name_dashes = module + '-' + check_name
   filename = os.path.normpath(os.path.join(module_path,
                                            '../../docs/ReleaseNotes.rst'))
-  with open(filename, 'r') as f:
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   lineMatcher = re.compile('New checks')
   nextSectionMatcher = re.compile('New check aliases')
   checkMatcher = re.compile('- New :doc:`(.*)')
 
   print('Updating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     note_added = False
     header_found = False
     add_note_here = False
 
     for line in lines:
       if not note_added:
         match = lineMatcher.match(line)
         match_next = nextSectionMatcher.match(line)

 
 
 # Adds a test for the check.
 def write_test(module_path, module, check_name, test_extension):
   check_name_dashes = module + '-' + check_name
   filename = os.path.normpath(os.path.join(module_path, '../../test/clang-tidy/checkers',
                                            check_name_dashes + '.' + test_extension))
   print('Creating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     f.write("""// RUN: %%check_clang_tidy %%s %(check_name_dashes)s %%t
 
 // FIXME: Add something that triggers the check here.
 void f();
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [%(check_name_dashes)s]
 
 // FIXME: Verify the applied fix.
 //   * Make the CHECK patterns specific enough and try to make verified lines
def get_actual_filename(dirname, filename):
   return ""
 
 
 # Recreates the list of checks in the docs/clang-tidy/checks directory.
 def update_checks_list(clang_tidy_path):
   docs_dir = os.path.join(clang_tidy_path, '../docs/clang-tidy/checks')
   filename = os.path.normpath(os.path.join(docs_dir, 'list.rst'))
   # Read the content of the current list.rst file
-  with open(filename, 'r') as f:
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
   # Get all existing docs
   doc_files = list(filter(lambda s: s.endswith('.rst') and s != 'list.rst',
                      os.listdir(docs_dir)))
   doc_files.sort()
 
   def has_auto_fix(check_name):
     dirname, _, check_name = check_name.partition("-")
 
     checkerCode = get_actual_filename(dirname,
                                       get_camel_name(check_name) + '.cpp')
 
     if not os.path.isfile(checkerCode):
       return ""
 
-    with open(checkerCode) as f:
+    with io.open(checkerCode, encoding='utf8') as f:
       code = f.read()
       if 'FixItHint' in code or "ReplacementText" in code or "fixit" in code:
         # Some simple heuristics to figure out if a checker has an autofix or not.
         return ' "Yes"'
     return ""
 
   def process_doc(doc_file):
     check_name = doc_file.replace('.rst', '')
 
-    with open(os.path.join(docs_dir, doc_file), 'r') as doc:
+    with io.open(os.path.join(docs_dir, doc_file), 'r', encoding='utf8') as doc:
       content = doc.read()
       match = re.search('.*:orphan:.*', content)
 
       if match:
         # Orphan page, don't list it.
         return '', ''
 
       match = re.search('.*:http-equiv=refresh: \d+;URL=(.*).html.*',
if match and check_name:
         'autofix': has_auto_fix(match.group(1))
       }
     return ''
 
   checks = map(format_link, doc_files)
   checks_alias = map(format_link_alias, doc_files)
 
   print('Updating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     for line in lines:
       f.write(line)
       if line.strip() == ".. csv-table::":
         # We dump the checkers
         f.write('   :header: "Name", "Offers fixes"\n\n')
         f.writelines(checks)
         # and the aliases
         f.write('\n\n')
         f.write('.. csv-table:: Aliases..\n')
         f.write('   :header: "Name", "Redirect", "Offers fixes"\n\n')
         f.writelines(checks_alias)
         break
 
 
 # Adds a documentation for the check.
 def write_docs(module_path, module, check_name):
   check_name_dashes = module + '-' + check_name
   filename = os.path.normpath(os.path.join(
       module_path, '../../docs/clang-tidy/checks/', check_name_dashes + '.rst'))
   print('Creating %s...' % filename)
-  with open(filename, 'w') as f:
+  with io.open(filename, 'w', encoding='utf8') as f:
     f.write(""".. title:: clang-tidy - %(check_name_dashes)s
 
 %(check_name_dashes)s
 %(underline)s
 
 FIXME: Describe what patterns does the check detect and why. Give examples.
 """ % {'check_name_dashes': check_name_dashes,
        'underline': '=' * len(check_name_dashes)})

clang-tools-extra/clang-tidy/rename_check.py

 #!/usr/bin/env python
 #
 #===- rename_check.py - clang-tidy check renamer ------------*- python -*--===#
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===-----------------------------------------------------------------------===#
 
 import argparse
 import glob
+import io
 import os
 import re
 
-
 def replaceInFileRegex(fileName, sFrom, sTo):
   if sFrom == sTo:
     return
+
+  # The documentation files are encoded using UTF-8, however on Windows the
+  # default encoding might be different (e.g. CP-1252). To make sure UTF-8 is
+  # always used, use `io.open(filename, mode, encoding='utf8')` for reading and
+  # writing files here and elsewhere.
   txt = None
-  with open(fileName, "r") as f:
+  with io.open(fileName, 'r', encoding='utf8') as f:
     txt = f.read()
 
   txt = re.sub(sFrom, sTo, txt)
   print("Replacing '%s' -> '%s' in '%s'..." % (sFrom, sTo, fileName))
-  with open(fileName, "w") as f:
+  with io.open(fileName, 'w', encoding='utf8') as f:
     f.write(txt)
 
 
 def replaceInFile(fileName, sFrom, sTo):
   if sFrom == sTo:
     return
   txt = None
-  with open(fileName, "r") as f:
+  with io.open(fileName, 'r', encoding='utf8') as f:
     txt = f.read()
 
   if sFrom not in txt:
     return
 
   txt = txt.replace(sFrom, sTo)
   print("Replacing '%s' -> '%s' in '%s'..." % (sFrom, sTo, fileName))
-  with open(fileName, "w") as f:
+  with io.open(fileName, 'w', encoding='utf8') as f:
     f.write(txt)
 
 
 def generateCommentLineHeader(filename):
   return ''.join(['//===--- ',
                   os.path.basename(filename),
                   ' - clang-tidy ',
                   '-' * max(0, 42 - len(os.path.basename(filename))),
def fileRename(fileName, sFrom, sTo):
   newFileName = fileName.replace(sFrom, sTo)
   print("Renaming '%s' -> '%s'..." % (fileName, newFileName))
   os.rename(fileName, newFileName)
   return newFileName
 
 
 def deleteMatchingLines(fileName, pattern):
   lines = None
-  with open(fileName, "r") as f:
+  with io.open(fileName, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   not_matching_lines = [l for l in lines if not re.search(pattern, l)]
   if len(not_matching_lines) == len(lines):
     return False
 
   print("Removing lines matching '%s' in '%s'..." % (pattern, fileName))
   print('  ' + '  '.join([l for l in lines if re.search(pattern, l)]))
-  with open(fileName, "w") as f:
+  with io.open(fileName, 'w', encoding='utf8') as f:
     f.writelines(not_matching_lines)
 
   return True
 
 
 def getListOfFiles(clang_tidy_path):
   files = glob.glob(os.path.join(clang_tidy_path, '*'))
   for dirname in files:
     if os.path.isdir(dirname):
       files += glob.glob(os.path.join(dirname, '*'))
   files += glob.glob(os.path.join(clang_tidy_path, '..', 'test',
                                   'clang-tidy', '*'))
   files += glob.glob(os.path.join(clang_tidy_path, '..', 'docs',
                                   'clang-tidy', 'checks', '*'))
   return [filename for filename in files if os.path.isfile(filename)]
 
 
 # Adapts the module's CMakelist file. Returns 'True' if it could add a new
 # entry and 'False' if the entry already existed.
 def adapt_cmake(module_path, check_name_camel):
   filename = os.path.join(module_path, 'CMakeLists.txt')
-  with open(filename, 'r') as f:
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   cpp_file = check_name_camel + '.cpp'
 
   # Figure out whether this check already exists.
   for line in lines:
     if line.strip() == cpp_file:
       return False
 
   print('Updating %s...' % filename)
-  with open(filename, 'wb') as f:
+  with io.open(filename, 'wb', encoding='utf8') as f:
     cpp_found = False
     file_added = False
     for line in lines:
       cpp_line = line.strip().endswith('.cpp')
       if (not file_added) and (cpp_line or cpp_found):
         cpp_found = True
         if (line.strip() > cpp_file) or (not cpp_line):
           f.write(('  ' + cpp_file + '\n').encode())
           file_added = True
       f.write(line.encode())
 
   return True
 
 # Modifies the module to include the new check.
 def adapt_module(module_path, module, check_name, check_name_camel):
   modulecpp = next(filter(lambda p: p.lower() == module.lower() + 'tidymodule.cpp', os.listdir(module_path)))
   filename = os.path.join(module_path, modulecpp)
-  with open(filename, 'r') as f:
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   print('Updating %s...' % filename)
-  with open(filename, 'wb') as f:
+  with io.open(filename, 'wb', encoding='utf8') as f:
     header_added = False
     header_found = False
     check_added = False
     check_decl = ('    CheckFactories.registerCheck<' + check_name_camel +
                   '>(\n        "' + check_name + '");\n')
 
     for line in lines:
       if not header_added:
for line in lines:
             f.write(check_decl.encode())
       f.write(line.encode())
 
 
 # Adds a release notes entry.
 def add_release_notes(clang_tidy_path, old_check_name, new_check_name):
   filename = os.path.normpath(os.path.join(clang_tidy_path,
                                            '../docs/ReleaseNotes.rst'))
-  with open(filename, 'r') as f:
+  with io.open(filename, 'r', encoding='utf8') as f:
     lines = f.readlines()
 
   lineMatcher = re.compile('Renamed checks')
   nextSectionMatcher = re.compile('Improvements to include-fixer')
   checkMatcher = re.compile('- The \'(.*)')
 
   print('Updating %s...' % filename)
-  with open(filename, 'wb') as f:
+  with io.open(filename, 'wb', encoding='utf8') as f:
     note_added = False
     header_found = False
     add_note_here = False
 
     for line in lines:
       if not note_added:
         match = lineMatcher.match(line)
         match_next = nextSectionMatcher.match(line)