[Diag] Create frontend flags for localization and write tests #32568

HassanElDesouky · 2020-06-26T16:40:36Z

In the last PR #32483, I introduced localization support for diagnostics via file-per-language store in YAML format.

In this PR I'm introducing the following:

Retrieve the diagnostic messages path from the compiler toolchain.
Create a -locale compiler frontend flag for choosing the language.
Create a -localization-path compiler frontend flag to provide a path for the YAML files (to use in testing).
Write tests.

The tests handle these cases:

If the language locale code isn't supported. In this case, we won't crash the compiler but instead show an error message of the supported locale codes.
If the path provided to -localization-path is incorrect i.e. the directory is missing the YAML files. We won't crash the compiler but instead, we should fall back to the default messages from the current .def files.
If the current <lang>.yaml doesn't have a translation for a particular diagnostic message it should fall back to the English message found in .def files.
Finally, if the message is translated and available it should, of course, show the translated message.

In the future PRs, I should handle lint tools for YAML files and prune tools to prune the diagnostics that gets removed from .def files.

cc @xedin

HassanElDesouky · 2020-06-26T16:47:45Z

@LucianoPAlmeida in #32483 you were suggesting using an assertion rather than an if statement here:
https://github.com/apple/swift/blob/master/include/swift/AST/DiagnosticEngine.h#L743
Can you please, explain more why to do that?

lib/Frontend/CompilerInvocation.cpp

xedin · 2020-06-26T16:56:20Z

@HassanElDesouky I can just on that an tell you why - since it's expected that locale and path are always there when setLocalization is called it's usually good to validate that assertion with assert to make sure that it's always true :)

include/swift/AST/diagnostics/fr.yaml

xedin

Overall looks great! Let's get comments I have left inline addressed and this would be ready to merge.

lib/Frontend/CompilerInvocation.cpp

lib/Frontend/Frontend.cpp

include/swift/Basic/DiagnosticOptions.h

test/diagnostics/Localization/no_localization_files.swift

include/swift/AST/DiagnosticsFrontend.def

include/swift/AST/diagnostics/en.yaml

lib/Frontend/CompilerInvocation.cpp

include/swift/AST/diagnostics/fr.yaml

lib/Driver/ToolChains.cpp

lib/Frontend/CompilerInvocation.cpp

LucianoPAlmeida · 2020-06-27T03:07:05Z

@HassanElDesouky I can just on that an tell you why - since it's expected that locale and path are always there when setLocalization is called it's usually good to validate that assertion with assert to make sure that it's always true :)

Exactly how @xedin said :)
It is a good way to make it easier for the caller to spot the error if it passed the wrong value by mistake. In this case, just silent not setting the locale based on an implementation detail condition not known by the caller, could lead to a "why is not setting?" and some time after that trying to figure out.

lib/Frontend/CompilerInvocation.cpp

include/swift/AST/LocalizationLanguages.def

lib/Frontend/Frontend.cpp

test/diagnostics/Localization/no_localization_files_and_wrong_path.swift

include/swift/AST/DiagnosticsFrontend.def

include/swift/AST/LocalizationLanguages.def

test/diagnostics/Localization/Inputs/en.yaml

include/swift/AST/LocalizationLanguages.def

lib/Frontend/CompilerInvocation.cpp

include/swift/AST/DefineLocalizationMacros.h

include/swift/AST/LocalizationLanguages.def

lib/Frontend/CompilerInvocation.cpp

include/swift/AST/DiagnosticEngine.h

include/swift/AST/LocalizationLanguages.def

xedin

Everything is coming along now! I have left just a couple of comments inline and once addressed we should be ready!

lib/Frontend/CompilerInvocation.cpp

include/swift/AST/DiagnosticsFrontend.def

lib/Frontend/CompilerInvocation.cpp

xedin

LGTM!

xedin · 2020-06-30T03:31:34Z

@swift-ci please test

swift-ci · 2020-06-30T03:31:59Z

Build failed
Swift Test Linux Platform
Git Sha - 935ac0c8e7a8f48a7eae241ee3acf5898961fdd7

swift-ci · 2020-06-30T03:36:03Z

Build failed
Swift Test OS X Platform
Git Sha - 935ac0c8e7a8f48a7eae241ee3acf5898961fdd7

davezarzycki · 2020-06-30T12:31:06Z

I'm going to need to revert this. The driver is double faulting on my Linux box (Fedora 32 / x86-64). It crashes due to heap corruption, then hangs trying to introspect and print the stack. There also appears to be an unrelated(?) uninitialized memory error that valgrind detects (as opposed to malloc's own self diagnostics):

 $ env SDKROOT=/ valgrind '/home/dave/b/q/t/bin/swift' -module-cache-path '/home/dave/b/q/t/swift-test-results/x86_64-unknown-linux-gnu/clang-module-cache' -swift-version 4  -Xfrontend -ignore-module-source-info -locale NOT_A_LOCALE
==2090498== Memcheck, a memory error detector
==2090498== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2090498== Using Valgrind-3.16.0 and LibVEX; rerun with -h for copyright info
==2090498== Command: /home/dave/b/q/t/bin/swift -module-cache-path /home/dave/b/q/t/swift-test-results/x86_64-unknown-linux-gnu/clang-module-cache -swift-version 4 -Xfrontend -ignore-module-source-info -locale NOT_A_LOCALE
==2090498==
==2090498== Syscall param execve(envp[i]) points to uninitialised byte(s)
==2090498==    at 0x983B45B: execve (in /usr/lib64/libc-2.31.so)
==2090498==    by 0x256386A: swift::driver::Compilation::performSingleCommand(swift::driver::Job const*) (in /home/dave/b/q/t/bin/swift-frontend)
==2090498==    by 0x256411A: swift::driver::Compilation::performJobs(std::__1::unique_ptr<swift::sys::TaskQueue, std::__1::default_delete<swift::sys::TaskQueue> >&&) (in /home/dave/b/q/t/bin/swift-frontend)
==2090498==    by 0x2550584: run_driver(llvm::StringRef, llvm::ArrayRef<char const*>) (in /home/dave/b/q/t/bin/swift-frontend)
==2090498==    by 0x254F547: main (in /home/dave/b/q/t/bin/swift-frontend)
==2090498==  Address 0x72ea02f is in the brk data segment 0x72ea000-0x72ea02f
==2090498==
malloc(): invalid next size (unsorted)
Stack dump:
0.      Program arguments: /home/dave/b/q/t/bin/lldb --repl=-disable-objc-interop -sdk / -color-diagnostics -module-cache-path /home/dave/b/q/t/swift-test-results/x86_64-unknown-linux-gnu/clang-module-cache -swift-version 4 -locale NOT_A_LOCALE -ignore-module-source-info
^C
$

That being said, if I bypass the driver, it works.

…swiftlang#32568)" This reverts commit 621b3b4. The driver is double faulting on my Linux box (Fedora 32 / x86-64). It crashes due to heap corruption, then hangs trying to introspect and print the stack. There also appears to be an unrelated(?) uninitialized memory error that valgrind detects (as opposed to malloc's own self diagnostics).

DougGregor · 2020-06-30T19:30:15Z

lib/Frontend/CompilerInvocation.cpp

+
+      Diags.diagnose(SourceLoc(), diag::error_invalid_locale_code,
+                     availableLocaleCodes);
+      return true;


This should not return after erroring. It should just ignore the bogus -locale

theblixguy reviewed Jun 26, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

theblixguy reviewed Jun 26, 2020

View reviewed changes

include/swift/AST/diagnostics/fr.yaml Show resolved Hide resolved

xedin reviewed Jun 26, 2020

View reviewed changes

LucianoPAlmeida reviewed Jun 27, 2020

View reviewed changes

lib/Driver/ToolChains.cpp Show resolved Hide resolved

LucianoPAlmeida reviewed Jun 27, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

HassanElDesouky commented Jun 28, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

HassanElDesouky commented Jun 28, 2020

View reviewed changes

include/swift/AST/LocalizationLanguages.def Outdated Show resolved Hide resolved

HassanElDesouky commented Jun 28, 2020

View reviewed changes

lib/Frontend/Frontend.cpp Show resolved Hide resolved

LucianoPAlmeida reviewed Jun 28, 2020

View reviewed changes

test/diagnostics/Localization/no_localization_files_and_wrong_path.swift Outdated Show resolved Hide resolved

xedin requested changes Jun 29, 2020

View reviewed changes

include/swift/AST/DiagnosticsFrontend.def Show resolved Hide resolved

include/swift/AST/DiagnosticsFrontend.def Show resolved Hide resolved

include/swift/AST/LocalizationLanguages.def Outdated Show resolved Hide resolved

test/diagnostics/Localization/Inputs/en.yaml Show resolved Hide resolved

HassanElDesouky commented Jun 29, 2020

View reviewed changes

include/swift/AST/LocalizationLanguages.def Outdated Show resolved Hide resolved

HassanElDesouky commented Jun 29, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Show resolved Hide resolved

HassanElDesouky commented Jun 29, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

HassanElDesouky requested review from xedin and LucianoPAlmeida June 29, 2020 16:04

xedin reviewed Jun 29, 2020

View reviewed changes

include/swift/AST/DefineLocalizationMacros.h Outdated Show resolved Hide resolved

HassanElDesouky commented Jun 29, 2020

View reviewed changes

include/swift/AST/LocalizationLanguages.def Outdated Show resolved Hide resolved

LucianoPAlmeida reviewed Jun 29, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

xedin requested changes Jun 29, 2020

View reviewed changes

include/swift/AST/DiagnosticEngine.h Outdated Show resolved Hide resolved

include/swift/AST/LocalizationLanguages.def Outdated Show resolved Hide resolved

xedin reviewed Jun 29, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

LucianoPAlmeida reviewed Jun 30, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Show resolved Hide resolved

xedin reviewed Jun 30, 2020

View reviewed changes

include/swift/AST/DiagnosticsFrontend.def Outdated Show resolved Hide resolved

Create frontend flags for localization

d9789d9

HassanElDesouky force-pushed the LocaleCompilerFlags branch from 917d4e5 to d9789d9 Compare June 30, 2020 02:04

HassanElDesouky commented Jun 30, 2020

View reviewed changes

lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

Remove assertion

ff2a221

xedin approved these changes Jun 30, 2020

View reviewed changes

xedin merged commit 621b3b4 into swiftlang:master Jun 30, 2020

davezarzycki mentioned this pull request Jun 30, 2020

Revert "[Diag] Create frontend flags for localization and write tests #32615

Merged

DougGregor reviewed Jun 30, 2020

View reviewed changes

HassanElDesouky mentioned this pull request Jun 30, 2020

Reopen [Diag] Create frontend flags for localization #32630

Merged

[Diag] Create frontend flags for localization and write tests #32568

[Diag] Create frontend flags for localization and write tests #32568

Uh oh!

Conversation

HassanElDesouky commented Jun 26, 2020

Uh oh!

HassanElDesouky commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

xedin commented Jun 26, 2020

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented Jun 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 30, 2020

Uh oh!

swift-ci commented Jun 30, 2020

Uh oh!

swift-ci commented Jun 30, 2020

Uh oh!

davezarzycki commented Jun 30, 2020

Uh oh!

DougGregor Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

xedin Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HassanElDesouky commented Jun 26, 2020 •

edited

Loading